Date:   Mon, 06 Mar 2023 14:40:55 +0530
Message-Id: <87ttyy1bz4.fsf@doe.com>
From:   Ritesh Harjani (IBM) <ritesh.list@gmail.com>
To:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        Theodore Tso <tytso@mit.edu>,
        Andreas Dilger <adilger.kernel@dilger.ca>
Cc:     "Matthew Wilcox (Oracle)" <willy@infradead.org>,
        linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 04/31] ext4: Convert ext4_finish_bio() to use folios
In-Reply-To: <20230126202415.1682629-5-willy@infradead.org>
Precedence: bulk

"Matthew Wilcox (Oracle)" <willy@infradead.org> writes:

> Prepare ext4 to support large folios in the page writeback path.

Sure. I am guessing for ext4 to completely support large folio
requires more work like fscrypt bounce page handling doesn't
yet support folios right?

Could you please give a little background on what all would be required
to add large folio support in ext4 buffered I/O path?
(I mean ofcourse other than saying move ext4 to iomap ;))

What I was interested in was, what other components in particular for
e.g. fscrypt, fsverity, ext4's xyz component needs large folio support?

And how should one go about in adding this support? So can we move
ext4's read path to have large folio support to get started?
Have you already identified what all is missing from this path to
convert it?

> Also set the actual error in the mapping, not just -EIO.

Right. I looked at the history and I think it always just had EIO.
I think setting the actual err in mapping_set_error() is the right thing
to do here.

>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

w.r.t this patch series. I reviewed the mechanical changes & error paths
which converts ext4 ext4_finish_bio() to use folio.

The changes looks good to me from that perspective. Feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>


> ---
>  fs/ext4/page-io.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index 982791050892..fd6c0dca24b9 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -99,30 +99,30 @@ static void buffer_io_error(struct buffer_head *bh)
>
>  static void ext4_finish_bio(struct bio *bio)
>  {
> -	struct bio_vec *bvec;
> -	struct bvec_iter_all iter_all;
> +	struct folio_iter fi;
>
> -	bio_for_each_segment_all(bvec, bio, iter_all) {
> -		struct page *page = bvec->bv_page;
> -		struct page *bounce_page = NULL;
> +	bio_for_each_folio_all(fi, bio) {
> +		struct folio *folio = fi.folio;
> +		struct folio *io_folio = NULL;
>  		struct buffer_head *bh, *head;
> -		unsigned bio_start = bvec->bv_offset;
> -		unsigned bio_end = bio_start + bvec->bv_len;
> +		size_t bio_start = fi.offset;
> +		size_t bio_end = bio_start + fi.length;
>  		unsigned under_io = 0;
>  		unsigned long flags;
>
> -		if (fscrypt_is_bounce_page(page)) {
> -			bounce_page = page;
> -			page = fscrypt_pagecache_page(bounce_page);
> +		if (fscrypt_is_bounce_folio(folio)) {
> +			io_folio = folio;
> +			folio = fscrypt_pagecache_folio(folio);
>  		}
>
>  		if (bio->bi_status) {
> -			SetPageError(page);
> -			mapping_set_error(page->mapping, -EIO);
> +			int err = blk_status_to_errno(bio->bi_status);
> +			folio_set_error(folio);
> +			mapping_set_error(folio->mapping, err);
>  		}
> -		bh = head = page_buffers(page);
> +		bh = head = folio_buffers(folio);
>  		/*
> -		 * We check all buffers in the page under b_uptodate_lock
> +		 * We check all buffers in the folio under b_uptodate_lock
>  		 * to avoid races with other end io clearing async_write flags
>  		 */
>  		spin_lock_irqsave(&head->b_uptodate_lock, flags);
> @@ -141,8 +141,8 @@ static void ext4_finish_bio(struct bio *bio)
>  		} while ((bh = bh->b_this_page) != head);
>  		spin_unlock_irqrestore(&head->b_uptodate_lock, flags);
>  		if (!under_io) {
> -			fscrypt_free_bounce_page(bounce_page);
> -			end_page_writeback(page);
> +			fscrypt_free_bounce_page(&io_folio->page);

Could you please help understand what would it take to convert bounce
page in fscrypt to folio?

Today, we allocate 32 bounce pages of order 0 via mempool in
    fscrypt_initialize()
    <...>
        fscrypt_bounce_page_pool =
            mempool_create_page_pool(num_prealloc_crypto_pages, 0);
    <...>

And IIUC, we might need to add some support for having higher order
pages in the pool so that one can allocate a folio->_folio_order
folio from this pool for bounce page to support large folio.
Is that understanding correct? Your thoughts on this please?

-ritesh