diff mbox series

[v2,5/7] nilfs2: Convert nilfs_find_uncommited_extent() to use filemap_get_folios_contig()

Message ID 20220816175246.42401-6-vishal.moola@gmail.com (mailing list archive)
State New, archived
Headers show
Series Convert to filemap_get_folios_contig() | expand

Commit Message

Vishal Moola (Oracle) Aug. 16, 2022, 5:52 p.m. UTC
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(-)

Comments

Ryusuke Konishi Aug. 17, 2022, 4:33 a.m. UTC | #1
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
>
Vishal Moola (Oracle) Aug. 17, 2022, 8:35 p.m. UTC | #2
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 mbox series

Patch

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;
 }