| Message ID | 20200717102240.21742-1-robbieko@synology.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: fix memory leak for page count | expand |
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]); > err = -EAGAIN; > goto out; > } > -- > 2.17.1 >
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 >> > >
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 >> >
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; }