[RFC PATCH 0/3] convert page_endio to folio_endio

[RFC PATCH 0/3] convert page_endio to folio_endio

[Pankaj Raghav]

page_endio() already works on folios by converting a page in to a folio as
the first step. This small series converts page_endio() to take a folio as
the first parameter instead of a page, and use native folio API at the
callee site to simplify the call to the converted folio_endio()
function.

mpage changes were tested with a simple boot testing. zram and orangefs is
only build tested. No functional changes were introduced as a part of
this AFAIK.

Pankaj Raghav (3):
  filemap: convert page_endio to folio_endio
  mpage: use bio_for_each_folio_all in mpage_end_io()
  orangefs: use folio in orangefs_readahead()

 drivers/block/zram/zram_drv.c |  2 +-
 fs/mpage.c                    | 11 ++++-------
 fs/orangefs/inode.c           |  8 ++++----
 include/linux/pagemap.h       |  2 +-
 mm/filemap.c                  |  8 +++-----
 5 files changed, 13 insertions(+), 18 deletions(-)

-- 
2.34.1

[RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Pankaj Raghav]

page_endio() already works on folios by converting a page in to a folio as
the first step. Convert page_endio to folio_endio by taking a folio as the
first parameter.

Instead of doing a page to folio conversion in the page_endio()
function, the consumers of this API do this conversion and call the
folio_endio() function in this patch.
The following patches will convert the consumers of this API to use native
folio functions to pass to folio_endio().

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 drivers/block/zram/zram_drv.c | 2 +-
 fs/mpage.c                    | 2 +-
 fs/orangefs/inode.c           | 2 +-
 include/linux/pagemap.h       | 2 +-
 mm/filemap.c                  | 8 +++-----
 5 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index aa490da3cef2..f441251c9138 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -610,7 +610,7 @@ static void zram_page_end_io(struct bio *bio)
 {
 	struct page *page = bio_first_page_all(bio);
 
-	page_endio(page, op_is_write(bio_op(bio)),
+	folio_endio(page_folio(page), op_is_write(bio_op(bio)),
 			blk_status_to_errno(bio->bi_status));
 	bio_put(bio);
 }
diff --git a/fs/mpage.c b/fs/mpage.c
index 22b9de5ddd68..40e86e839e77 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -50,7 +50,7 @@ static void mpage_end_io(struct bio *bio)
 
 	bio_for_each_segment_all(bv, bio, iter_all) {
 		struct page *page = bv->bv_page;
-		page_endio(page, bio_op(bio),
+		folio_endio(page_folio(page), bio_op(bio),
 			   blk_status_to_errno(bio->bi_status));
 	}
 
diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index aefdf1d3be7c..b12d099510ea 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -276,7 +276,7 @@ static void orangefs_readahead(struct readahead_control *rac)
 
 	/* clean up. */
 	while ((page = readahead_page(rac))) {
-		page_endio(page, false, ret);
+		folio_endio(page_folio(page), false, ret);
 		put_page(page);
 	}
 }
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index fdcd595d2294..80eab64b834f 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -1076,7 +1076,7 @@ int filemap_migrate_folio(struct address_space *mapping, struct folio *dst,
 #else
 #define filemap_migrate_folio NULL
 #endif
-void page_endio(struct page *page, bool is_write, int err);
+void folio_endio(struct folio *folio, bool is_write, int err);
 
 void folio_end_private_2(struct folio *folio);
 void folio_wait_private_2(struct folio *folio);
diff --git a/mm/filemap.c b/mm/filemap.c
index a34abfe8c654..a89940f74974 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1626,13 +1626,11 @@ void folio_end_writeback(struct folio *folio)
 EXPORT_SYMBOL(folio_end_writeback);
 
 /*
- * After completing I/O on a page, call this routine to update the page
+ * After completing I/O on a folio, call this routine to update the folio
  * flags appropriately
  */
-void page_endio(struct page *page, bool is_write, int err)
+void folio_endio(struct folio *folio, bool is_write, int err)
 {
-	struct folio *folio = page_folio(page);
-
 	if (!is_write) {
 		if (!err) {
 			folio_mark_uptodate(folio);
@@ -1653,7 +1651,7 @@ void page_endio(struct page *page, bool is_write, int err)
 		folio_end_writeback(folio);
 	}
 }
-EXPORT_SYMBOL_GPL(page_endio);
+EXPORT_SYMBOL_GPL(folio_endio);
 
 /**
  * __folio_lock - Get a lock on the folio, assuming we need to sleep to get it.
-- 
2.34.1

[RFC PATCH 2/3] mpage: use bio_for_each_folio_all in mpage_end_io()

[Pankaj Raghav]

Use bio_for_each_folio_all to iterate through folios in a bio so that
the folios can be directly passed to the folio_endio() function.

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 fs/mpage.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/fs/mpage.c b/fs/mpage.c
index 40e86e839e77..bfcc139938a8 100644
--- a/fs/mpage.c
+++ b/fs/mpage.c
@@ -45,14 +45,11 @@
  */
 static void mpage_end_io(struct bio *bio)
 {
-	struct bio_vec *bv;
-	struct bvec_iter_all iter_all;
+	struct folio_iter fi;
 
-	bio_for_each_segment_all(bv, bio, iter_all) {
-		struct page *page = bv->bv_page;
-		folio_endio(page_folio(page), bio_op(bio),
-			   blk_status_to_errno(bio->bi_status));
-	}
+	bio_for_each_folio_all(fi, bio)
+		folio_endio(fi.folio, bio_op(bio),
+			    blk_status_to_errno(bio->bi_status));
 
 	bio_put(bio);
 }
-- 
2.34.1

[RFC PATCH 3/3] orangefs: use folio in orangefs_readahead()

[Pankaj Raghav]

Use folio and its corresponding function in orangefs_readahead() so that
folios can be directly passed to the folio_endio().

Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
 fs/orangefs/inode.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
index b12d099510ea..7e03d60bd406 100644
--- a/fs/orangefs/inode.c
+++ b/fs/orangefs/inode.c
@@ -244,7 +244,7 @@ static void orangefs_readahead(struct readahead_control *rac)
 	struct iov_iter iter;
 	struct inode *inode = rac->mapping->host;
 	struct xarray *i_pages;
-	struct page *page;
+	struct folio *folio;
 	loff_t new_start = readahead_pos(rac);
 	int ret;
 	size_t new_len = 0;
@@ -275,9 +275,9 @@ static void orangefs_readahead(struct readahead_control *rac)
 		ret = 0;
 
 	/* clean up. */
-	while ((page = readahead_page(rac))) {
-		folio_endio(page_folio(page), false, ret);
-		put_page(page);
+	while ((folio = readahead_folio(rac))) {
+		folio_endio(folio, false, ret);
+		folio_put(folio);
 	}
 }
 
-- 
2.34.1

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Luis Chamberlain]

On Wed, Mar 15, 2023 at 01:32:31PM +0100, Pankaj Raghav wrote:
> page_endio() already works on folios by converting a page in to a folio as
> the first step. Convert page_endio to folio_endio by taking a folio as the
> first parameter.
> 
> Instead of doing a page to folio conversion in the page_endio()
> function, the consumers of this API do this conversion and call the
> folio_endio() function in this patch.
> The following patches will convert the consumers of this API to use native
> folio functions to pass to folio_endio().
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

Perhaps the hard thing is what tree would this go through?

  Luis

Re: [RFC PATCH 2/3] mpage: use bio_for_each_folio_all in mpage_end_io()

[Luis Chamberlain]

On Wed, Mar 15, 2023 at 01:32:32PM +0100, Pankaj Raghav wrote:
> Use bio_for_each_folio_all to iterate through folios in a bio so that
> the folios can be directly passed to the folio_endio() function.
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [RFC PATCH 3/3] orangefs: use folio in orangefs_readahead()

[Luis Chamberlain]

On Wed, Mar 15, 2023 at 01:32:33PM +0100, Pankaj Raghav wrote:
> Use folio and its corresponding function in orangefs_readahead() so that
> folios can be directly passed to the folio_endio().
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>

Reviewed-by: Luis Chamberlain <mcgrof@kernel.org>

  Luis

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Hannes Reinecke]

On 3/15/23 13:32, Pankaj Raghav wrote:
> page_endio() already works on folios by converting a page in to a folio as
> the first step. Convert page_endio to folio_endio by taking a folio as the
> first parameter.
> 
> Instead of doing a page to folio conversion in the page_endio()
> function, the consumers of this API do this conversion and call the
> folio_endio() function in this patch.
> The following patches will convert the consumers of this API to use native
> folio functions to pass to folio_endio().
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
>   drivers/block/zram/zram_drv.c | 2 +-
>   fs/mpage.c                    | 2 +-
>   fs/orangefs/inode.c           | 2 +-
>   include/linux/pagemap.h       | 2 +-
>   mm/filemap.c                  | 8 +++-----
>   5 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index aa490da3cef2..f441251c9138 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -610,7 +610,7 @@ static void zram_page_end_io(struct bio *bio)
>   {
>   	struct page *page = bio_first_page_all(bio);
>   
> -	page_endio(page, op_is_write(bio_op(bio)),
> +	folio_endio(page_folio(page), op_is_write(bio_op(bio)),
>   			blk_status_to_errno(bio->bi_status));
>   	bio_put(bio);
>   }
> diff --git a/fs/mpage.c b/fs/mpage.c
> index 22b9de5ddd68..40e86e839e77 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -50,7 +50,7 @@ static void mpage_end_io(struct bio *bio)
>   
>   	bio_for_each_segment_all(bv, bio, iter_all) {
>   		struct page *page = bv->bv_page;
> -		page_endio(page, bio_op(bio),
> +		folio_endio(page_folio(page), bio_op(bio),
>   			   blk_status_to_errno(bio->bi_status));
>   	}
>   
Can't this be converted to use 'bio_for_each_folio_all()' instead of
bio_for_each_segment_all()?

Cheers,

Hannes

Re: [RFC PATCH 2/3] mpage: use bio_for_each_folio_all in mpage_end_io()

[Hannes Reinecke]

On 3/15/23 13:32, Pankaj Raghav wrote:
> Use bio_for_each_folio_all to iterate through folios in a bio so that
> the folios can be directly passed to the folio_endio() function.
> 
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
>   fs/mpage.c | 11 ++++-------
>   1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/mpage.c b/fs/mpage.c
> index 40e86e839e77..bfcc139938a8 100644
> --- a/fs/mpage.c
> +++ b/fs/mpage.c
> @@ -45,14 +45,11 @@
>    */
>   static void mpage_end_io(struct bio *bio)
>   {
> -	struct bio_vec *bv;
> -	struct bvec_iter_all iter_all;
> +	struct folio_iter fi;
>   
> -	bio_for_each_segment_all(bv, bio, iter_all) {
> -		struct page *page = bv->bv_page;
> -		folio_endio(page_folio(page), bio_op(bio),
> -			   blk_status_to_errno(bio->bi_status));
> -	}
> +	bio_for_each_folio_all(fi, bio)
> +		folio_endio(fi.folio, bio_op(bio),
> +			    blk_status_to_errno(bio->bi_status));
>   
>   	bio_put(bio);
>   }

Ah. Here it is.

I would suggest merge these two patches.

Cheers,

Hannes

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Christoph Hellwig]

Can we take a step back and figure out if page_endio is a good
idea to start with?

The zram usage seems clearly wrong to me.  zram is a block driver
and does not own the pages, so it shouldn't touch any of the page
state.  It seems like this mostly operates on it's own
pages allocated using alloc_page so the harm might not be horrible
at least.

orangefs uses it on readahead pages, with ret known for the whole
iteration.  So one quick loop for the success and one for the
failure case would look simpler an more obvious.

mpage really should use separate end_io handler for read vs write
as well like most other aops do.

So overall I'd be happier to just kill the helper.

Re: [RFC PATCH 3/3] orangefs: use folio in orangefs_readahead()

[Matthew Wilcox]

On Wed, Mar 15, 2023 at 01:32:33PM +0100, Pankaj Raghav wrote:
> Use folio and its corresponding function in orangefs_readahead() so that
> folios can be directly passed to the folio_endio().

This is wrong; you need to drop the call to folio_put().

>  	/* clean up. */
> -	while ((page = readahead_page(rac))) {
> -		folio_endio(page_folio(page), false, ret);
> -		put_page(page);
> +	while ((folio = readahead_folio(rac))) {
> +		folio_endio(folio, false, ret);
> +		folio_put(folio);
>  	}

Re: [RFC PATCH 2/3] mpage: use bio_for_each_folio_all in mpage_end_io()

[Matthew Wilcox]

On Wed, Mar 15, 2023 at 03:52:15PM +0100, Hannes Reinecke wrote:
> On 3/15/23 13:32, Pankaj Raghav wrote:
> > Use bio_for_each_folio_all to iterate through folios in a bio so that
> > the folios can be directly passed to the folio_endio() function.
> > +	bio_for_each_folio_all(fi, bio)
> > +		folio_endio(fi.folio, bio_op(bio),
> > +			    blk_status_to_errno(bio->bi_status));
> >   	bio_put(bio);
> >   }
> 
> Ah. Here it is.
> 
> I would suggest merge these two patches.

The right way to have handled this patch series was:

1. Introduce a new folio_endio() [but see Christoph's mail on why we
shouldn't do that]
2-n convert callers to use folios directly
n+1 remove page_endio() entirely.

Note that patch n+1 might not be part of this patch series; sometimes
it takes a while to convert all callers to use folios.

I very much dislike the way this was done by pushing the page_folio()
call into each of the callers because it makes the entire series hard to
review.

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Pankaj Raghav]

Hi Christoph,

On 2023-03-15 15:56, Christoph Hellwig wrote:
> Can we take a step back and figure out if page_endio is a good
> idea to start with?
> 
> The zram usage seems clearly wrong to me.  zram is a block driver
> and does not own the pages, so it shouldn't touch any of the page
> state.  It seems like this mostly operates on it's own
> pages allocated using alloc_page so the harm might not be horrible
> at least.
> 
It looks like this endio function is called when alloc_page is used (for
partial IO) to trigger writeback from the user space `echo "idle" >
/sys/block/zram0/writeback`.

I don't understand when you say the harm might not be horrible if we don't
call folio_endio here. Do you think it is just safe to remove the call to
folio_endio function?

> orangefs uses it on readahead pages, with ret known for the whole
> iteration.  So one quick loop for the success and one for the
> failure case would look simpler an more obvious.
> 
Got it. Something like this?

@@ -266,18 +266,23 @@ static void orangefs_readahead(struct
readahead_control *rac)
        iov_iter_xarray(&iter, ITER_DEST, i_pages, offset,
readahead_length(rac));

        /* read in the pages. */
-       if ((ret = wait_for_direct_io(ORANGEFS_IO_READ, inode,
-                       &offset, &iter, readahead_length(rac),
-                       inode->i_size, NULL, NULL, rac->file)) < 0)
+       if ((ret = wait_for_direct_io(ORANGEFS_IO_READ, inode, &offset, &iter,
+                                     readahead_length(rac), inode->i_size,
+                                     NULL, NULL, rac->file)) < 0) {
                gossip_debug(GOSSIP_FILE_DEBUG,
-                       "%s: wait_for_direct_io failed. \n", __func__);
-       else
-               ret = 0;
+                            "%s: wait_for_direct_io failed. \n", __func__);

-       /* clean up. */
-       while ((page = readahead_page(rac))) {
-               page_endio(page, false, ret);
-               put_page(page);
+               while ((folio = readahead_folio(rac))) {
+                       folio_clear_uptodate(folio);
+                       folio_set_error(folio);
+                       folio_unlock(folio);
+               }
+               return;
+       }
+
+       while ((folio = readahead_folio(rac))) {
+               folio_mark_uptodate(folio);
+               folio_unlock(folio);
        }
 }

> mpage really should use separate end_io handler for read vs write
> as well like most other aops do.
> 

I don't know if this is the right abstraction to do the switch, but let me
know what you think:
@@ -59,7 +59,8 @@ static void mpage_end_io(struct bio *bio)
static struct bio *mpage_bio_submit(struct bio *bio)
 {
-       bio->bi_end_io = mpage_end_io;
+       bio->bi_end_io = (op_is_write(bio_op(bio))) ? mpage_write_end_io :
+                                                     mpage_read_end_io;
        guard_bio_eod(bio);
        submit_bio(bio);
        return NULL;
And mpage_{write,read}_end_io will iterate over the folio and call the
respective functions.

> So overall I'd be happier to just kill the helper.

Re: [RFC PATCH 2/3] mpage: use bio_for_each_folio_all in mpage_end_io()

[Pankaj Raghav]

On 2023-03-15 17:08, Matthew Wilcox wrote:
> On Wed, Mar 15, 2023 at 03:52:15PM +0100, Hannes Reinecke wrote:
>> On 3/15/23 13:32, Pankaj Raghav wrote:
>>> Use bio_for_each_folio_all to iterate through folios in a bio so that
>>> the folios can be directly passed to the folio_endio() function.
>>> +	bio_for_each_folio_all(fi, bio)
>>> +		folio_endio(fi.folio, bio_op(bio),
>>> +			    blk_status_to_errno(bio->bi_status));
>>>   	bio_put(bio);
>>>   }
>>
>> Ah. Here it is.
>>
>> I would suggest merge these two patches.
> 
> The right way to have handled this patch series was:
> 
> 1. Introduce a new folio_endio() [but see Christoph's mail on why we
> shouldn't do that]
> 2-n convert callers to use folios directly
> n+1 remove page_endio() entirely.
> 
> Note that patch n+1 might not be part of this patch series; sometimes
> it takes a while to convert all callers to use folios.
> 

This is definitely a much better way to handle these changes. Thanks willy.

> I very much dislike the way this was done by pushing the page_folio()
> call into each of the callers because it makes the entire series hard to
> review.

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Christoph Hellwig]

On Thu, Mar 16, 2023 at 11:04:54AM +0100, Pankaj Raghav wrote:
> It looks like this endio function is called when alloc_page is used (for
> partial IO) to trigger writeback from the user space `echo "idle" >
> /sys/block/zram0/writeback`.


Yes.

> I don't understand when you say the harm might not be horrible if we don't
> call folio_endio here. Do you think it is just safe to remove the call to
> folio_endio function?

I suspect so.  It doesn't seem like the involved pages are ever locked
or have the writeback set, so it should be fine.

> +               while ((folio = readahead_folio(rac))) {
> +                       folio_clear_uptodate(folio);
> +                       folio_set_error(folio);
> +                       folio_unlock(folio);
> +               }
> +               return;
> +       }
> +
> +       while ((folio = readahead_folio(rac))) {
> +               folio_mark_uptodate(folio);
> +               folio_unlock(folio);
>         }
>  }

Looks good.

> @@ -59,7 +59,8 @@ static void mpage_end_io(struct bio *bio)
> static struct bio *mpage_bio_submit(struct bio *bio)
>  {
> -       bio->bi_end_io = mpage_end_io;
> +       bio->bi_end_io = (op_is_write(bio_op(bio))) ? mpage_write_end_io :
> +                                                     mpage_read_end_io;
>         guard_bio_eod(bio);
>         submit_bio(bio);
>         return NULL;
> And mpage_{write,read}_end_io will iterate over the folio and call the
> respective functions.

Yes, although I'd do it with a good old if/else and with less braces.
It might make sense to split mpage_bio_submit as well, though.

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Matthew Wilcox]

On Thu, Mar 16, 2023 at 11:04:54AM +0100, Pankaj Raghav wrote:
> -       /* clean up. */
> -       while ((page = readahead_page(rac))) {
> -               page_endio(page, false, ret);
> -               put_page(page);
> +               while ((folio = readahead_folio(rac))) {
> +                       folio_clear_uptodate(folio);
> +                       folio_set_error(folio);
> +                       folio_unlock(folio);
> +               }
> +               return;
> +       }
> +
> +       while ((folio = readahead_folio(rac))) {
> +               folio_mark_uptodate(folio);
> +               folio_unlock(folio);
>         }

readahead_folio() is a bit too heavy-weight for that, IMO.  I'd do this
as;

	while ((folio = readahead_folio(rac))) {
		if (!ret)
			folio_mark_uptodate(folio);
		folio_unlock(folio);
	}

(there's no need to call folio_set_error(), nor folio_clear_uptodate())

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Pankaj Raghav]

On 2023-03-17 16:31, Matthew Wilcox wrote:
>> +
>> +       while ((folio = readahead_folio(rac))) {
>> +               folio_mark_uptodate(folio);
>> +               folio_unlock(folio);
>>         }
> 
> readahead_folio() is a bit too heavy-weight for that, IMO.  I'd do this
> as;
> 
> 	while ((folio = readahead_folio(rac))) {
> 		if (!ret)
> 			folio_mark_uptodate(folio);
> 		folio_unlock(folio);
> 	}
> 

This looks good.

> (there's no need to call folio_set_error(), nor folio_clear_uptodate())

I am trying to understand why these calls are not needed for the error case.
I see similar pattern, for e.g. in iomap_finish_folio_read() where we call these
functions for the error case.

If we don't need to call these anymore, can the mpage code also be shortened like this:

-static void mpage_end_io(struct bio *bio)
+static void mpage_read_end_io(struct bio *bio)
 {
-       struct bio_vec *bv;
-       struct bvec_iter_all iter_all;
+       struct folio_iter fi;
+       int err = blk_status_to_errno(bio->bi_status);

-       bio_for_each_segment_all(bv, bio, iter_all) {
-               struct page *page = bv->bv_page;
-               page_endio(page, bio_op(bio),
-                          blk_status_to_errno(bio->bi_status));
+       bio_for_each_folio_all(fi, bio) {
+               struct folio *folio = fi.folio;
+
+               if (!err)
+                       folio_mark_uptodate(folio);
+               folio_unlock(folio);
+       }
+
+       bio_put(bio);
+}
+
+static void mpage_write_end_io(struct bio *bio)
+{
....
+

Re: [RFC PATCH 1/3] filemap: convert page_endio to folio_endio

[Pankaj Raghav]

Hi Willy,

On 2023-03-17 17:14, Pankaj Raghav wrote:
> On 2023-03-17 16:31, Matthew Wilcox wrote:
>>> +
>>> +       while ((folio = readahead_folio(rac))) {
>>> +               folio_mark_uptodate(folio);
>>> +               folio_unlock(folio);
>>>         }
>>
>> readahead_folio() is a bit too heavy-weight for that, IMO.  I'd do this
>> as;
>>
>> 	while ((folio = readahead_folio(rac))) {
>> 		if (!ret)
>> 			folio_mark_uptodate(folio);
>> 		folio_unlock(folio);
>> 	}
>>
> 
> This looks good.
> 
>> (there's no need to call folio_set_error(), nor folio_clear_uptodate())
> 
> I am trying to understand why these calls are not needed for the error case.
> I see similar pattern, for e.g. in iomap_finish_folio_read() where we call these
> functions for the error case.
> 

I am planning to send the next version. It would be great if I can get a rationale for your
statement regarding not needing to call folio_set_error() or folio_clear_uptodate().

> If we don't need to call these anymore, can the mpage code also be shortened like this:
> 
> -static void mpage_end_io(struct bio *bio)
> +static void mpage_read_end_io(struct bio *bio)
>  {
> -       struct bio_vec *bv;
> -       struct bvec_iter_all iter_all;
> +       struct folio_iter fi;
> +       int err = blk_status_to_errno(bio->bi_status);
> 
> -       bio_for_each_segment_all(bv, bio, iter_all) {
> -               struct page *page = bv->bv_page;
> -               page_endio(page, bio_op(bio),
> -                          blk_status_to_errno(bio->bi_status));
> +       bio_for_each_folio_all(fi, bio) {
> +               struct folio *folio = fi.folio;
> +
> +               if (!err)
> +                       folio_mark_uptodate(folio);
> +               folio_unlock(folio);
> +       }
> +
> +       bio_put(bio);
> +}
> +
> +static void mpage_write_end_io(struct bio *bio)
> +{
> ....
> +