| Message ID | 20220816175246.42401-6-vishal.moola@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Convert to filemap_get_folios_contig() | expand |
On Wed, Aug 17, 2022 at 2:54 AM Vishal Moola (Oracle) wrote: > > Converted function to use folios throughout. This is in preparation for > the removal of find_get_pages_contig(). Now also supports large folios. > > Also cleaned up an unnecessary if statement - pvec.pages[0]->index > index > will always evaluate to false, and filemap_get_folios_contig() returns 0 if > there is no folio found at index. > > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> > --- > > v2: > - Fixed a warning regarding a now unused label "out" > - Reported-by: kernel test robot <lkp@intel.com> > --- > fs/nilfs2/page.c | 39 +++++++++++++++++---------------------- > 1 file changed, 17 insertions(+), 22 deletions(-) > > diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c > index 3267e96c256c..14629e03d0da 100644 > --- a/fs/nilfs2/page.c > +++ b/fs/nilfs2/page.c > @@ -480,13 +480,13 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, > sector_t start_blk, > sector_t *blkoff) > { > - unsigned int i; > + unsigned int i, nr; > pgoff_t index; > unsigned int nblocks_in_page; > unsigned long length = 0; > sector_t b; > - struct pagevec pvec; > - struct page *page; > + struct folio_batch fbatch; > + struct folio *folio; > > if (inode->i_mapping->nrpages == 0) > return 0; > @@ -494,27 +494,24 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, > index = start_blk >> (PAGE_SHIFT - inode->i_blkbits); > nblocks_in_page = 1U << (PAGE_SHIFT - inode->i_blkbits); > > - pagevec_init(&pvec); > + folio_batch_init(&fbatch); > > repeat: > - pvec.nr = find_get_pages_contig(inode->i_mapping, index, PAGEVEC_SIZE, > - pvec.pages); > - if (pvec.nr == 0) > + nr = filemap_get_folios_contig(inode->i_mapping, &index, ULONG_MAX, > + &fbatch); > + if (nr == 0) > return length; > > - if (length > 0 && pvec.pages[0]->index > index) > - goto out; > - > - b = pvec.pages[0]->index << (PAGE_SHIFT - inode->i_blkbits); > + b = fbatch.folios[0]->index << (PAGE_SHIFT - inode->i_blkbits); > i = 0; > do { > - page = pvec.pages[i]; > + folio = fbatch.folios[i]; > > - lock_page(page); > - if (page_has_buffers(page)) { > + folio_lock(folio); > + if (folio_buffers(folio)) { > struct buffer_head *bh, *head; > > - bh = head = page_buffers(page); > + bh = head = folio_buffers(folio); > do { > if (b < start_blk) > continue; > @@ -532,18 +529,16 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, > > b += nblocks_in_page; Here, It looks like the block index "b" should be updated with the number of blocks in the folio because the loop is now per folio, not per page. Instead of replacing it with a calculation that uses folio_size(folio) or folio_shift(folio), I think it would be better to move the calculation of "b" inside the branch where the folio has buffers as follows: if (folio_buffers(folio)) { struct buffer_head *bh, *head; sector_t b; b = folio->index << (PAGE_SHIFT - inode->i_blkbits); bh = head = folio_buffers(folio); ... } else if (length > 0) { goto out_locked; } This way, we can remove calculations for the block index "b" outside the above part and the variable "nblocks_in_page" as well. Thanks, Ryusuke Konishi > } > - unlock_page(page); > + folio_unlock(folio); > > - } while (++i < pagevec_count(&pvec)); > + } while (++i < nr); > > - index = page->index + 1; > - pagevec_release(&pvec); > + folio_batch_release(&fbatch); > cond_resched(); > goto repeat; > > out_locked: > - unlock_page(page); > -out: > - pagevec_release(&pvec); > + folio_unlock(folio); > + folio_batch_release(&fbatch); > return length; > } > -- > 2.36.1 >
On Tue, Aug 16, 2022 at 9:33 PM Ryusuke Konishi <konishi.ryusuke@gmail.com> wrote: > > On Wed, Aug 17, 2022 at 2:54 AM Vishal Moola (Oracle) wrote: > > > > Converted function to use folios throughout. This is in preparation for > > the removal of find_get_pages_contig(). Now also supports large folios. > > > > Also cleaned up an unnecessary if statement - pvec.pages[0]->index > index > > will always evaluate to false, and filemap_get_folios_contig() returns 0 if > > there is no folio found at index. > > > > Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> > > --- > > > > v2: > > - Fixed a warning regarding a now unused label "out" > > - Reported-by: kernel test robot <lkp@intel.com> > > --- > > fs/nilfs2/page.c | 39 +++++++++++++++++---------------------- > > 1 file changed, 17 insertions(+), 22 deletions(-) > > > > diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c > > index 3267e96c256c..14629e03d0da 100644 > > --- a/fs/nilfs2/page.c > > +++ b/fs/nilfs2/page.c > > @@ -480,13 +480,13 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, > > sector_t start_blk, > > sector_t *blkoff) > > { > > - unsigned int i; > > + unsigned int i, nr; > > pgoff_t index; > > unsigned int nblocks_in_page; > > unsigned long length = 0; > > sector_t b; > > - struct pagevec pvec; > > - struct page *page; > > + struct folio_batch fbatch; > > + struct folio *folio; > > > > if (inode->i_mapping->nrpages == 0) > > return 0; > > @@ -494,27 +494,24 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, > > index = start_blk >> (PAGE_SHIFT - inode->i_blkbits); > > nblocks_in_page = 1U << (PAGE_SHIFT - inode->i_blkbits); > > > > - pagevec_init(&pvec); > > + folio_batch_init(&fbatch); > > > > repeat: > > - pvec.nr = find_get_pages_contig(inode->i_mapping, index, PAGEVEC_SIZE, > > - pvec.pages); > > - if (pvec.nr == 0) > > + nr = filemap_get_folios_contig(inode->i_mapping, &index, ULONG_MAX, > > + &fbatch); > > + if (nr == 0) > > return length; > > > > - if (length > 0 && pvec.pages[0]->index > index) > > - goto out; > > - > > - b = pvec.pages[0]->index << (PAGE_SHIFT - inode->i_blkbits); > > + b = fbatch.folios[0]->index << (PAGE_SHIFT - inode->i_blkbits); > > i = 0; > > do { > > - page = pvec.pages[i]; > > + folio = fbatch.folios[i]; > > > > - lock_page(page); > > - if (page_has_buffers(page)) { > > + folio_lock(folio); > > + if (folio_buffers(folio)) { > > struct buffer_head *bh, *head; > > > > - bh = head = page_buffers(page); > > + bh = head = folio_buffers(folio); > > do { > > if (b < start_blk) > > continue; > > @@ -532,18 +529,16 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, > > > > > b += nblocks_in_page; > > Here, It looks like the block index "b" should be updated with the > number of blocks in the > folio because the loop is now per folio, not per page. Yup, thanks for catching that. > Instead of replacing it with a calculation that uses folio_size(folio) > or folio_shift(folio), > I think it would be better to move the calculation of "b" inside the > branch where the folio > has buffers as follows: > > if (folio_buffers(folio)) { > struct buffer_head *bh, *head; > sector_t b; > > b = folio->index << (PAGE_SHIFT - inode->i_blkbits); > bh = head = folio_buffers(folio); > ... > } else if (length > 0) { > goto out_locked; > } > > This way, we can remove calculations for the block index "b" outside > the above part > and the variable "nblocks_in_page" as well. Sounds good, I'll do that. > Thanks, > Ryusuke Konishi > > > } > > - unlock_page(page); > > + folio_unlock(folio); > > > > - } while (++i < pagevec_count(&pvec)); > > + } while (++i < nr); > > > > - index = page->index + 1; > > - pagevec_release(&pvec); > > + folio_batch_release(&fbatch); > > cond_resched(); > > goto repeat; > > > > out_locked: > > - unlock_page(page); > > -out: > > - pagevec_release(&pvec); > > + folio_unlock(folio); > > + folio_batch_release(&fbatch); > > return length; > > } > > -- > > 2.36.1 > >
diff --git a/fs/nilfs2/page.c b/fs/nilfs2/page.c index 3267e96c256c..14629e03d0da 100644 --- a/fs/nilfs2/page.c +++ b/fs/nilfs2/page.c @@ -480,13 +480,13 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, sector_t start_blk, sector_t *blkoff) { - unsigned int i; + unsigned int i, nr; pgoff_t index; unsigned int nblocks_in_page; unsigned long length = 0; sector_t b; - struct pagevec pvec; - struct page *page; + struct folio_batch fbatch; + struct folio *folio; if (inode->i_mapping->nrpages == 0) return 0; @@ -494,27 +494,24 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, index = start_blk >> (PAGE_SHIFT - inode->i_blkbits); nblocks_in_page = 1U << (PAGE_SHIFT - inode->i_blkbits); - pagevec_init(&pvec); + folio_batch_init(&fbatch); repeat: - pvec.nr = find_get_pages_contig(inode->i_mapping, index, PAGEVEC_SIZE, - pvec.pages); - if (pvec.nr == 0) + nr = filemap_get_folios_contig(inode->i_mapping, &index, ULONG_MAX, + &fbatch); + if (nr == 0) return length; - if (length > 0 && pvec.pages[0]->index > index) - goto out; - - b = pvec.pages[0]->index << (PAGE_SHIFT - inode->i_blkbits); + b = fbatch.folios[0]->index << (PAGE_SHIFT - inode->i_blkbits); i = 0; do { - page = pvec.pages[i]; + folio = fbatch.folios[i]; - lock_page(page); - if (page_has_buffers(page)) { + folio_lock(folio); + if (folio_buffers(folio)) { struct buffer_head *bh, *head; - bh = head = page_buffers(page); + bh = head = folio_buffers(folio); do { if (b < start_blk) continue; @@ -532,18 +529,16 @@ unsigned long nilfs_find_uncommitted_extent(struct inode *inode, b += nblocks_in_page; } - unlock_page(page); + folio_unlock(folio); - } while (++i < pagevec_count(&pvec)); + } while (++i < nr); - index = page->index + 1; - pagevec_release(&pvec); + folio_batch_release(&fbatch); cond_resched(); goto repeat; out_locked: - unlock_page(page); -out: - pagevec_release(&pvec); + folio_unlock(folio); + folio_batch_release(&fbatch); return length; }
Converted function to use folios throughout. This is in preparation for the removal of find_get_pages_contig(). Now also supports large folios. Also cleaned up an unnecessary if statement - pvec.pages[0]->index > index will always evaluate to false, and filemap_get_folios_contig() returns 0 if there is no folio found at index. Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com> --- v2: - Fixed a warning regarding a now unused label "out" - Reported-by: kernel test robot <lkp@intel.com> --- fs/nilfs2/page.c | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-)