Date:   Mon, 06 Mar 2023 12:21:48 +0530
Message-Id: <87wn3u1iez.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 25/31] ext4: Convert ext4_block_write_begin() to take a folio
In-Reply-To: <20230126202415.1682629-26-willy@infradead.org>
Precedence: bulk

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

> All the callers now have a folio, so pass that in and operate on folios.
> Removes four calls to compound_head().

Why do you say four? Isn't it 3 calls of PageUptodate(page) which
removes calls to compound_head()? Which one did I miss?

>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  fs/ext4/inode.c | 41 +++++++++++++++++++++--------------------
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index dbfc0670de75..507c7f88d737 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1055,12 +1055,12 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
>  }
>
>  #ifdef CONFIG_FS_ENCRYPTION
> -static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
> +static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
>  				  get_block_t *get_block)
>  {
>  	unsigned from = pos & (PAGE_SIZE - 1);
>  	unsigned to = from + len;
> -	struct inode *inode = page->mapping->host;
> +	struct inode *inode = folio->mapping->host;
>  	unsigned block_start, block_end;
>  	sector_t block;
>  	int err = 0;
> @@ -1070,22 +1070,24 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
>  	int nr_wait = 0;
>  	int i;
>
> -	BUG_ON(!PageLocked(page));
> +	BUG_ON(!folio_test_locked(folio));
>  	BUG_ON(from > PAGE_SIZE);
>  	BUG_ON(to > PAGE_SIZE);
>  	BUG_ON(from > to);
>
> -	if (!page_has_buffers(page))
> -		create_empty_buffers(page, blocksize, 0);
> -	head = page_buffers(page);
> +	head = folio_buffers(folio);
> +	if (!head) {
> +		create_empty_buffers(&folio->page, blocksize, 0);
> +		head = folio_buffers(folio);
> +	}
>  	bbits = ilog2(blocksize);
> -	block = (sector_t)page->index << (PAGE_SHIFT - bbits);
> +	block = (sector_t)folio->index << (PAGE_SHIFT - bbits);
>
>  	for (bh = head, block_start = 0; bh != head || !block_start;
>  	    block++, block_start = block_end, bh = bh->b_this_page) {
>  		block_end = block_start + blocksize;
>  		if (block_end <= from || block_start >= to) {
> -			if (PageUptodate(page)) {
> +			if (folio_test_uptodate(folio)) {
>  				set_buffer_uptodate(bh);
>  			}
>  			continue;
> @@ -1098,19 +1100,20 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
>  			if (err)
>  				break;
>  			if (buffer_new(bh)) {
> -				if (PageUptodate(page)) {
> +				if (folio_test_uptodate(folio)) {
>  					clear_buffer_new(bh);
>  					set_buffer_uptodate(bh);
>  					mark_buffer_dirty(bh);
>  					continue;
>  				}
>  				if (block_end > to || block_start < from)
> -					zero_user_segments(page, to, block_end,
> -							   block_start, from);
> +					folio_zero_segments(folio, to,
> +							    block_end,
> +							    block_start, from);
>  				continue;
>  			}
>  		}
> -		if (PageUptodate(page)) {
> +		if (folio_test_uptodate(folio)) {
>  			set_buffer_uptodate(bh);
>  			continue;
>  		}
> @@ -1130,13 +1133,13 @@ static int ext4_block_write_begin(struct page *page, loff_t pos, unsigned len,
>  			err = -EIO;
>  	}
>  	if (unlikely(err)) {
> -		page_zero_new_buffers(page, from, to);
> +		page_zero_new_buffers(&folio->page, from, to);
>  	} else if (fscrypt_inode_uses_fs_layer_crypto(inode)) {
>  		for (i = 0; i < nr_wait; i++) {
>  			int err2;
>
> -			err2 = fscrypt_decrypt_pagecache_blocks(page, blocksize,
> -								bh_offset(wait[i]));
> +			err2 = fscrypt_decrypt_pagecache_blocks(&folio->page,
> +						blocksize, bh_offset(wait[i]));

folio_decrypt_pagecache_blocks() takes folio as it's argument now.

Other than that it looks good to me. Please feel free to add -
Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>