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

From: Robbie Ko <robbieko@synology.com>
To: fdmanana@gmail.com
Cc: linux-btrfs <linux-btrfs@vger.kernel.org>
Subject: Re: [PATCH] btrfs: fix memory leak for page count
Date: Mon, 20 Jul 2020 09:27:25 +0800	[thread overview]
Message-ID: <86cdd766-3488-20fa-caf9-037b5bd05308@synology.com> (raw)
In-Reply-To: <CAL3q7H4+yzwLm6yhSfmSLbh6d00geGQsM-h6TiZzgAQHWT+yiQ@mail.gmail.com>

Filipe Manana 於 2020/7/17 下午8:00 寫道:
> 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.

OK, I will resend patch.

Thanks.

>> 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]);
>>                                          err = -EAGAIN;
>>                                          goto out;
>>                                  }
>> --
>> 2.17.1
>>
>