Date:   Sun, 05 Mar 2023 17:56:32 +0530
Message-Id: <87zg8r1j0n.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 20/31] ext4: Convert __ext4_block_zero_page_range() to use a folio
In-Reply-To: <20230126202415.1682629-21-willy@infradead.org>
Precedence: bulk

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

> Use folio APIs throughout.  Saves many calls to compound_head().

minor comment below.

>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/ext4/inode.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index b79e591b7c8e..727aa2e51a9d 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3812,23 +3812,26 @@ static int __ext4_block_zero_page_range(handle_t *handle,
>  	ext4_lblk_t iblock;
>  	struct inode *inode = mapping->host;
>  	struct buffer_head *bh;
> -	struct page *page;
> +	struct folio *folio;
>  	int err = 0;
>
> -	page = find_or_create_page(mapping, from >> PAGE_SHIFT,
> -				   mapping_gfp_constraint(mapping, ~__GFP_FS));
> -	if (!page)
> +	folio = __filemap_get_folio(mapping, from >> PAGE_SHIFT,
> +				    FGP_LOCK | FGP_ACCESSED | FGP_CREAT,
> +				    mapping_gfp_constraint(mapping, ~__GFP_FS));
> +	if (!folio)
>  		return -ENOMEM;
>
>  	blocksize = inode->i_sb->s_blocksize;
>
>  	iblock = index << (PAGE_SHIFT - inode->i_sb->s_blocksize_bits);
>
> -	if (!page_has_buffers(page))
> -		create_empty_buffers(page, blocksize, 0);
> +	bh = folio_buffers(folio);
> +	if (!bh) {
> +		create_empty_buffers(&folio->page, blocksize, 0);
> +		bh = folio_buffers(folio);
> +	}
>
>  	/* Find the buffer that contains "offset" */
> -	bh = page_buffers(page);
>  	pos = blocksize;
>  	while (offset >= pos) {
>  		bh = bh->b_this_page;
> @@ -3850,7 +3853,7 @@ static int __ext4_block_zero_page_range(handle_t *handle,
>  	}
>
>  	/* Ok, it's mapped. Make sure it's up-to-date */
> -	if (PageUptodate(page))
> +	if (folio_test_uptodate(folio))
>  		set_buffer_uptodate(bh);
>
>  	if (!buffer_uptodate(bh)) {
> @@ -3860,7 +3863,8 @@ static int __ext4_block_zero_page_range(handle_t *handle,
>  		if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
>  			/* We expect the key to be set. */
>  			BUG_ON(!fscrypt_has_encryption_key(inode));
> -			err = fscrypt_decrypt_pagecache_blocks(page, blocksize,
> +			err = fscrypt_decrypt_pagecache_blocks(&folio->page,
> +							       blocksize,
>  							       bh_offset(bh));

I think after this patch which added support for decrypting large folio,
fscrypt_descrypt_pagecache_blocks() takes folio as it's 1st argument.
Hence this patch will need a small change to pass folio instead of page.

Other than that the change looks good to me.

Please feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

    51e4e3153ebc32d3280d5d17418ae6f1a44f1ec1
    Author:     Eric Biggers <ebiggers@google.com>
    CommitDate: Sat Jan 28 15:10:12 2023 -0800

    fscrypt: support decrypting data from large folios


-ritesh