On 17.07.20 г. 15:00 ч., Filipe Manana wrote:
> 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]);
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
>>
>
>
