On Fri, Jul 17, 2020 at 11:23 AM robbieko <robbieko@xxxxxxxxxxxx> wrote:
>
> From: Robbie Ko <robbieko@xxxxxxxxxxxx>
>
> When lock_delalloc_page, we first lock the page and then
> check that the page dirty, if the page is not dirty, we
> will return -EAGAIN but all pages must be freed, otherwise
> page leak.
"When lock_delalloc_page" -> When locking pages for delalloc
We check if it's dirty and if the mapping still matches.
Btw, you can make line length closer to 75 characters, it makes things
a bit more readable.
The subject is also a bit confusing:
"btrfs: fix memory leak for page count"
something along the lines "btrfs: fix page leaks after failure to lock
page for delalloc" would be more clear to me at least,
it gives a clue about where the problem is.
>
> Signed-off-by: Robbie Ko <robbieko@xxxxxxxxxxxx>
The code looks correct, thanks.
Reviewed-by: Filipe Manana <fdmanana@xxxxxxxx>
> ---
> fs/btrfs/extent_io.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 68c96057ad2d..34d55b1e2a88 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -1951,7 +1951,7 @@ static int __process_pages_contig(struct address_space *mapping,
> struct page *pages[16];
> unsigned ret;
> int err = 0;
> - int i;
> + int i, j;
>
> if (page_ops & PAGE_LOCK) {
> ASSERT(page_ops == PAGE_LOCK);
> @@ -1999,7 +1999,8 @@ static int __process_pages_contig(struct address_space *mapping,
> if (!PageDirty(pages[i]) ||
> pages[i]->mapping != mapping) {
> unlock_page(pages[i]);
> - put_page(pages[i]);
> + for (j = i; j < ret; j++)
> + put_page(pages[j]);
> err = -EAGAIN;
> goto out;
> }
> --
> 2.17.1
>
--
Filipe David Manana,
“Whether you think you can, or you think you can't — you're right.”
