Re: [PATCH 09/10] ext4: move zero partial block range functions out of active handle

[Jan Kara]

On Tue 10-03-26 09:41:00, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> Move ext4_block_zero_eof() and ext4_zero_partial_blocks() calls out of
> the active handle context, making them independent operations. This is
> safe because it still ensures data is updated before metadata for
> data=ordered mode and data=journal mode because we still zero data and
> ordering data before modifying the metadata.
> 
> This change is required for iomap infrastructure conversion because the
> iomap buffered I/O path does not use the same journal infrastructure for
> partial block zeroing. The lock ordering of folio lock and starting
> transactions is "folio lock -> transaction start", which is opposite of
> the current path. Therefore, zeroing partial blocks cannot be performed
> under the active handle.
> 
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>

So in this patch you also move ext4_zero_partial_blocks() before the
transaction start in ext4_zero_range() and ext4_punch_hole(). However
cannot these operations in theory fail with error such as EDQUOT or ENOSPC
when they need to grow the extent tree as a result of the operation? In
that case we'd return such error but partial blocks are already zeroed out
which is a problem... OTOH in these cases we don't need to order the data
at all so in principle we shouldn't need to move the zeroing earlier
here. Am I missing something?

								Honza

> @@ -4722,25 +4724,18 @@ static long ext4_zero_range(struct file *file, loff_t offset,
>  	if (IS_ALIGNED(offset | end, blocksize))
>  		return ret;
>  
> -	/*
> -	 * In worst case we have to writeout two nonadjacent unwritten
> -	 * blocks and update the inode
> -	 */
> -	credits = (2 * ext4_ext_index_trans_blocks(inode, 2)) + 1;
> -	if (ext4_should_journal_data(inode))
> -		credits += 2;
> -	handle = ext4_journal_start(inode, EXT4_HT_MISC, credits);
> +	/* Zero out partial block at the edges of the range */
> +	ret = ext4_zero_partial_blocks(inode, offset, len);
> +	if (ret)
> +		return ret;
> +
> +	handle = ext4_journal_start(inode, EXT4_HT_MISC, 1);
>  	if (IS_ERR(handle)) {
>  		ret = PTR_ERR(handle);
>  		ext4_std_error(inode->i_sb, ret);
>  		return ret;
>  	}
>  
> -	/* Zero out partial block at the edges of the range */
> -	ret = ext4_zero_partial_blocks(inode, offset, len);
> -	if (ret)
> -		goto out_handle;
> -
>  	if (new_size)
>  		ext4_update_inode_size(inode, new_size);
>  	ret = ext4_mark_inode_dirty(handle, inode);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index d5b783a7c814..5288d36b0f09 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -4443,8 +4443,12 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>  	if (ret)
>  		return ret;
>  
> +	ret = ext4_zero_partial_blocks(inode, offset, length);
> +	if (ret)
> +		return ret;
> +
>  	if (ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS))
> -		credits = ext4_chunk_trans_extent(inode, 2);
> +		credits = ext4_chunk_trans_extent(inode, 0);
>  	else
>  		credits = ext4_blocks_for_truncate(inode);
>  	handle = ext4_journal_start(inode, EXT4_HT_TRUNCATE, credits);
> @@ -4454,10 +4458,6 @@ int ext4_punch_hole(struct file *file, loff_t offset, loff_t length)
>  		return ret;
>  	}
>  
> -	ret = ext4_zero_partial_blocks(inode, offset, length);
> -	if (ret)
> -		goto out_handle;
> -
>  	/* If there are blocks to remove, do it */
>  	start_lblk = EXT4_B_TO_LBLK(inode, offset);
>  	end_lblk = end >> inode->i_blkbits;
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR