On Tue, Aug 16, 2022 at 9:33 PM Ryusuke Konishi
<konishi.ryusuke@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> >
> > v2:
> > - Fixed a warning regarding a now unused label "out"
> > - Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > ---
> > 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
> >
