[1/6] mm/page_io: use a folio in __end_swap_bio_read()

Commit Message

Peng Zhang July 17, 2023, 1:25 p.m. UTC

From: ZhangPeng <[e-mail]>

Saves three implicit call to compound_head().

Signed-off-by: ZhangPeng <[e-mail]>
---
 mm/page_io.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Matthew Wilcox (Oracle) July 17, 2023, 1:34 p.m. UTC | #1

On Mon, Jul 17, 2023 at 09:25:57PM +0800, Peng Zhang wrote:
> +++ b/mm/page_io.c
> @@ -58,18 +58,18 @@ static void end_swap_bio_write(struct bio *bio)
>  
>  static void __end_swap_bio_read(struct bio *bio)
>  {
> -	struct page *page = bio_first_page_all(bio);
> +	struct folio *folio = page_folio(bio_first_page_all(bio));

Should we have a bio_first_folio_all()?  It wouldn't save any calls to
compound_head(), but it's slightly easier to use.

>  	if (bio->bi_status) {
> -		SetPageError(page);
> -		ClearPageUptodate(page);
> +		folio_set_error(folio);

I appreciate this is a 1:1 conversion, but maybe we could think about
this a bit.  Is there anybody who checks the
PageError()/folio_test_error() for this page/folio?

> +		folio_clear_uptodate(folio);

Similarly ... surely the folio is already !uptodate, so we don't need to
clear the flag?

Peng Zhang July 18, 2023, 12:56 p.m. UTC | #2

On 2023/7/17 21:34, Matthew Wilcox wrote:

> On Mon, Jul 17, 2023 at 09:25:57PM +0800, Peng Zhang wrote:
>> +++ b/mm/page_io.c
>> @@ -58,18 +58,18 @@ static void end_swap_bio_write(struct bio *bio)
>>   
>>   static void __end_swap_bio_read(struct bio *bio)
>>   {
>> -	struct page *page = bio_first_page_all(bio);
>> +	struct folio *folio = page_folio(bio_first_page_all(bio));
> Should we have a bio_first_folio_all()?  It wouldn't save any calls to
> compound_head(), but it's slightly easier to use.

Yes, I'll convert bio_first_page_all() to bio_first_folio_all() in a v2.
Thanks.

>>   	if (bio->bi_status) {
>> -		SetPageError(page);
>> -		ClearPageUptodate(page);
>> +		folio_set_error(folio);
> I appreciate this is a 1:1 conversion, but maybe we could think about
> this a bit.  Is there anybody who checks the
> PageError()/folio_test_error() for this page/folio?

Maybe wait_dev_supers() checks the PageError() after write_dev_supers()
in fs/btrfs/disk-io.c?

>> +		folio_clear_uptodate(folio);
> Similarly ... surely the folio is already !uptodate, so we don't need to
> clear the flag?

Indeed, the folio is already !uptodate. I'll drop this line in a v2.
Thanks for your feedback.

Matthew Wilcox (Oracle) July 18, 2023, 4:16 p.m. UTC | #3

On Tue, Jul 18, 2023 at 08:56:16PM +0800, zhangpeng (AS) wrote:
> > >   	if (bio->bi_status) {
> > > -		SetPageError(page);
> > > -		ClearPageUptodate(page);
> > > +		folio_set_error(folio);
> > I appreciate this is a 1:1 conversion, but maybe we could think about
> > this a bit.  Is there anybody who checks the
> > PageError()/folio_test_error() for this page/folio?
> 
> Maybe wait_dev_supers() checks the PageError() after write_dev_supers()
> in fs/btrfs/disk-io.c?

How does _this_ folio end up in btrfs's write_dev_supers()?  This is a
swap read.  The only folios which are swapped are anonymous and tmpfs.
btrfs takes care of doing its own I/O.  wait_dev_supers() is looking
for the error set in btrfs_end_super_write() which is the completion
routine for write_dev_supers().  The pages involved there are attached
to a btrfs address_space, not shmem or anon.

Peng Zhang July 19, 2023, 7:46 a.m. UTC | #4

On 2023/7/19 0:16, Matthew Wilcox wrote:

> On Tue, Jul 18, 2023 at 08:56:16PM +0800, zhangpeng (AS) wrote:
>>>>    	if (bio->bi_status) {
>>>> -		SetPageError(page);
>>>> -		ClearPageUptodate(page);
>>>> +		folio_set_error(folio);
>>> I appreciate this is a 1:1 conversion, but maybe we could think about
>>> this a bit.  Is there anybody who checks the
>>> PageError()/folio_test_error() for this page/folio?
>> Maybe wait_dev_supers() checks the PageError() after write_dev_supers()
>> in fs/btrfs/disk-io.c?
> How does _this_ folio end up in btrfs's write_dev_supers()?  This is a
> swap read.  The only folios which are swapped are anonymous and tmpfs.
> btrfs takes care of doing its own I/O.  wait_dev_supers() is looking
> for the error set in btrfs_end_super_write() which is the completion
> routine for write_dev_supers().  The pages involved there are attached
> to a btrfs address_space, not shmem or anon.

Thanks for your explanation!

Then I think nobody checks the PageError()/folio_test_error() for the page
in patch 1 and patch 2. I'll delete SetPageError() in a v2.

Christoph Hellwig July 20, 2023, 5:21 a.m. UTC | #5

On Tue, Jul 18, 2023 at 05:16:15PM +0100, Matthew Wilcox wrote:
> How does _this_ folio end up in btrfs's write_dev_supers()?  This is a
> swap read.  The only folios which are swapped are anonymous and tmpfs.
> btrfs takes care of doing its own I/O.  wait_dev_supers() is looking
> for the error set in btrfs_end_super_write() which is the completion
> routine for write_dev_supers().  The pages involved there are attached
> to a btrfs address_space, not shmem or anon.

It actually operates on the block_device inode.  That does not matter
for this series, but complicates things in other ways.

Patch

diff --git a/mm/page_io.c b/mm/page_io.c
index 684cd3c7b59b..ebf431e5f538 100644
--- a/mm/page_io.c
+++ b/mm/page_io.c
@@ -58,18 +58,18 @@  static void end_swap_bio_write(struct bio *bio)
 
 static void __end_swap_bio_read(struct bio *bio)
 {
-	struct page *page = bio_first_page_all(bio);
+	struct folio *folio = page_folio(bio_first_page_all(bio));
 
 	if (bio->bi_status) {
-		SetPageError(page);
-		ClearPageUptodate(page);
+		folio_set_error(folio);
+		folio_clear_uptodate(folio);
 		pr_alert_ratelimited("Read-error on swap-device (%u:%u:%llu)\n",
 				     MAJOR(bio_dev(bio)), MINOR(bio_dev(bio)),
 				     (unsigned long long)bio->bi_iter.bi_sector);
 	} else {
-		SetPageUptodate(page);
+		folio_mark_uptodate(folio);
 	}
-	unlock_page(page);
+	folio_unlock(folio);
 }
 
 static void end_swap_bio_read(struct bio *bio)