Re: [PATCH] btrfs: fix memory leak for page count

[Nikolay Borisov <nborisov@suse.com>]

On 17.07.20 г. 15:00 ч., Filipe Manana wrote:
> On Fri, Jul 17, 2020 at 11:23 AM robbieko <robbieko@synology.com> wrote:
>>
>> From: Robbie Ko <robbieko@synology.com>
>>
>> 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@synology.com>
> 
> The code looks correct, thanks.
> 
> Reviewed-by: Filipe Manana <fdmanana@suse.com>
> 
>> ---
>>  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]);

nit: You don't need to introduce another variable, you can simply reuse
i from its current value:
for(; i < ret; i++)
  put_pages(pages[j]

If 'j' is to be used I'da rather have it defined in the 'if' branch so
it's lifespan is clearly visible.

>>                                         err = -EAGAIN;
>>                                         goto out;
>>                                 }
>> --
>> 2.17.1
>>
> 
>