Re: [PATCH 03/10] ext4: rename and extend ext4_block_truncate_page()

[Jan Kara]

On Tue 10-03-26 09:40:54, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Rename ext4_block_truncate_page() to ext4_block_zero_eof() and extend
> its signature to accept an explicit 'end' offset instead of calculating
> the block boundary. This helper function now can replace all cases
> requiring zeroing of the partial EOF block, including the append
> buffered write paths in ext4_*_write_end().
> 
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/ext4.h    |  2 ++
>  fs/ext4/extents.c |  4 ++--
>  fs/ext4/inode.c   | 43 +++++++++++++++++++++++--------------------
>  3 files changed, 27 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 293f698b7042..c62459ef9796 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3099,6 +3099,8 @@ extern int ext4_chunk_trans_blocks(struct inode *, int nrblocks);
>  extern int ext4_chunk_trans_extent(struct inode *inode, int nrblocks);
>  extern int ext4_meta_trans_blocks(struct inode *inode, int lblocks,
>  				  int pextents);
> +extern int ext4_block_zero_eof(handle_t *handle, struct inode *inode,
> +			       loff_t from, loff_t end);
>  extern int ext4_zero_partial_blocks(handle_t *handle, struct inode *inode,
>  			     loff_t lstart, loff_t lend);
>  extern vm_fault_t ext4_page_mkwrite(struct vm_fault *vmf);
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ae3804f36535..a265070c1b79 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4625,8 +4625,8 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset,
>  						      inode_get_ctime(inode));
>  			if (epos > old_size) {
>  				pagecache_isize_extended(inode, old_size, epos);
> -				ext4_zero_partial_blocks(handle, inode,
> -						     old_size, epos - old_size);
> +				ext4_block_zero_eof(handle, inode, old_size,
> +						    epos);
>  			}
>  		}
>  		ret2 = ext4_mark_inode_dirty(handle, inode);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index a737ce05e768..52c6a86ad9f9 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1458,7 +1458,7 @@ static int ext4_write_end(const struct kiocb *iocb,
>  
>  	if (old_size < pos && !verity) {
>  		pagecache_isize_extended(inode, old_size, pos);
> -		ext4_zero_partial_blocks(handle, inode, old_size, pos - old_size);
> +		ext4_block_zero_eof(handle, inode, old_size, pos);
>  	}
>  	/*
>  	 * Don't mark the inode dirty under folio lock. First, it unnecessarily
> @@ -1576,7 +1576,7 @@ static int ext4_journalled_write_end(const struct kiocb *iocb,
>  
>  	if (old_size < pos && !verity) {
>  		pagecache_isize_extended(inode, old_size, pos);
> -		ext4_zero_partial_blocks(handle, inode, old_size, pos - old_size);
> +		ext4_block_zero_eof(handle, inode, old_size, pos);
>  	}
>  
>  	if (size_changed) {
> @@ -3252,7 +3252,7 @@ static int ext4_da_do_write_end(struct address_space *mapping,
>  	if (IS_ERR(handle))
>  		return PTR_ERR(handle);
>  	if (zero_len)
> -		ext4_zero_partial_blocks(handle, inode, old_size, zero_len);
> +		ext4_block_zero_eof(handle, inode, old_size, pos);
>  	ext4_mark_inode_dirty(handle, inode);
>  	ext4_journal_stop(handle);
>  
> @@ -4132,29 +4132,32 @@ static int ext4_block_zero_page_range(handle_t *handle,
>  }
>  
>  /*
> - * ext4_block_truncate_page() zeroes out a mapping from file offset `from'
> - * up to the end of the block which corresponds to `from'.
> - * This required during truncate. We need to physically zero the tail end
> - * of that block so it doesn't yield old data if the file is later grown.
> - * Return the zeroed length on success.
> + * Zero out a mapping from file offset 'from' up to the end of the block
> + * which corresponds to 'from' or to the given 'end' inside this block.
> + * This required during truncate up and performing append writes. We need
> + * to physically zero the tail end of that block so it doesn't yield old
> + * data if the file is grown. Return the zeroed length on success.
>   */
> -static int ext4_block_truncate_page(handle_t *handle,
> -		struct address_space *mapping, loff_t from)
> +int ext4_block_zero_eof(handle_t *handle, struct inode *inode,
> +			loff_t from, loff_t end)
>  {
> -	unsigned length;
> -	unsigned blocksize;
> -	struct inode *inode = mapping->host;
> +	unsigned int blocksize = i_blocksize(inode);
> +	unsigned int offset;
> +	loff_t length = end - from;
>  	bool did_zero = false;
>  	int err;
>  
> +	offset = from & (blocksize - 1);
> +	if (!offset || from >= end)
> +		return 0;
>  	/* If we are processing an encrypted inode during orphan list handling */
>  	if (IS_ENCRYPTED(inode) && !fscrypt_has_encryption_key(inode))
>  		return 0;
>  
> -	blocksize = i_blocksize(inode);
> -	length = blocksize - (from & (blocksize - 1));
> +	if (length > blocksize - offset)
> +		length = blocksize - offset;
>  
> -	err = ext4_block_zero_page_range(handle, mapping, from, length,
> +	err = ext4_block_zero_page_range(handle, inode->i_mapping, from, length,
>  					 &did_zero);
>  	if (err)
>  		return err;
> @@ -4508,7 +4511,6 @@ int ext4_truncate(struct inode *inode)
>  	unsigned int credits;
>  	int err = 0, err2;
>  	handle_t *handle;
> -	struct address_space *mapping = inode->i_mapping;
>  
>  	/*
>  	 * There is a possibility that we're either freeing the inode
> @@ -4551,8 +4553,9 @@ int ext4_truncate(struct inode *inode)
>  		goto out_trace;
>  	}
>  
> +	/* Zero to the end of the block containing i_size */
>  	if (inode->i_size & (inode->i_sb->s_blocksize - 1))
> -		ext4_block_truncate_page(handle, mapping, inode->i_size);
> +		ext4_block_zero_eof(handle, inode, inode->i_size, LLONG_MAX);
>  
>  	/*
>  	 * We add the inode to the orphan list, so that if this
> @@ -5911,8 +5914,8 @@ int ext4_setattr(struct mnt_idmap *idmap, struct dentry *dentry,
>  				inode_set_mtime_to_ts(inode,
>  						      inode_set_ctime_current(inode));
>  				if (oldsize & (inode->i_sb->s_blocksize - 1))
> -					ext4_block_truncate_page(handle,
> -							inode->i_mapping, oldsize);
> +					ext4_block_zero_eof(handle, inode,
> +							    oldsize, LLONG_MAX);
>  			}
>  
>  			if (shrink)
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR