Re: [PATCH v2 3/4] ext4: add inode to orphan list before extent conversion

[Jan Kara]

On Fri 30-10-26 10:13:37, Zhang Yi wrote:
> From: Zhang Yi <yi.zhang@xxxxxxxxxx>
> 
> ext4_alloc_file_blocks() zeroes the blocks it has just allocated and
> converts their unwritten extents in a handle started with
> ext4_chunk_trans_blocks(), which reserves credits for a single extent.
> The range is allocated with ext4_map_blocks() first and converted
> afterwards, and only the unwritten extent conversion which runs on I/O
> completion can modify the extent tree of that range in between, since
> this path holds i_rwsem. If such a conversion converts part of the
> range first, it splits the extent, and converting the whole range then
> needs more credits than the handle has reserved.
> 
> Before commit 10660a0ca7ed ("ext4: rework handle credit accounting for
> unwritten extent conversion"), this race condition led to insufficient
> credits, which made the conversion fail with -ENOSPC in the middle of
> the range. After it, ext4_convert_unwritten_extents() can extend or
> restart the transaction. Restarting commits everything modified so
> far, so the extents converted by the first iterations reach the disk
> before this function adds the inode to the orphan list. A crash in
> between leaves written extents beyond i_disksize on disk with the
> inode not on the orphan list, which fsck complains about.
> 
> Add the inode to the orphan list before the conversion instead and keep
> it there until i_disksize has caught up. A restart then commits the
> orphan entry together with the extents converted so far, and the entry
> is only removed by the transaction which raises i_disksize and i_size.
> 
> Fixes: 10660a0ca7ed ("ext4: rework handle credit accounting for unwritten extent conversion")
> Fixes: f7237a775c8f ("ext4: protect WRITE_ZEROES written extents with orphan list")
> Signed-off-by: Zhang Yi <yi.zhang@xxxxxxxxxx>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <jack@xxxxxxx>

								Honza

> ---
>  fs/ext4/extents.c | 47 ++++++++++++++++++++++++++++++++---------------
>  1 file changed, 32 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index ad0d80e33f609..9f80beeaa1f29 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4701,6 +4701,23 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  				break;
>  			}
>  
> +			/*
> +			 * Add the inode to the orphan list before the
> +			 * conversion, as it may restart the transaction and
> +			 * commit the extents converted so far.  Written
> +			 * extents must stay covered by the orphan list until
> +			 * i_disksize catches up.
> +			 */
> +			if (new_size) {
> +				ret2 = ext4_orphan_add(handle, inode);
> +				orphan = true;
> +				if (unlikely(ret2)) {
> +					ret = ret2;
> +					ext4_journal_stop(handle);
> +					break;
> +				}
> +			}
> +
>  			ret = ext4_convert_unwritten_extents(handle,
>  					inode, (loff_t)map.m_lblk << blkbits,
>  					(loff_t)map.m_len << blkbits,
> @@ -4708,23 +4725,21 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  			if (ret)
>  				map.m_len = converted;
>  
> -			/*
> -			 * If blocks beyond i_disksize are converted, add
> -			 * the inode to the orphan list and advance the epos.
> -			 */
> -			if (new_size && converted) {
> -				ret2 = ext4_orphan_add(handle, inode);
> -				ret = ret ? ret : ret2;
> -				orphan = true;
> -			}
> -
>  			ret3 = ext4_journal_stop(handle);
>  			ret = ret ? ret : ret3;
>  		}
>  
> +		/*
> +		 * The conversion may have failed without converting
> +		 * anything. Only move epos over the blocks that were
> +		 * actually converted, so that a failure updates
> +		 * i_disksize just far enough to cover them.
> +		 */
> +		if (map.m_len)
> +			epos = EXT4_LBLK_TO_B(inode, map.m_lblk + map.m_len);
> +
>  		map.m_lblk += map.m_len;
>  		map.m_len = len_lblk = len_lblk - map.m_len;
> -		epos = EXT4_LBLK_TO_B(inode, map.m_lblk);
>  		if (ret)
>  			break;
>  	}
> @@ -4732,12 +4747,13 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>  		goto retry;
>  
> -	if (!epos || !new_size)
> +	if (!new_size || (!epos && !orphan))
>  		return ret;
>  
>  	/*
> -	 * Allocate blocks, update the file size to match the size of the
> -	 * already successfully allocated blocks.
> +	 * Update the file size to match the size of the already
> +	 * successfully allocated blocks, and release the orphan entry
> +	 * if we added one.
>  	 */
>  	if (epos > new_size)
>  		epos = new_size;
> @@ -4755,7 +4771,8 @@ static int ext4_alloc_file_blocks(struct file *file, loff_t offset, loff_t len,
>  		goto out;
>  	}
>  
> -	ext4_update_inode_size(inode, epos);
> +	if (epos)
> +		ext4_update_inode_size(inode, epos);
>  	if (orphan && inode->i_nlink)
>  		ext4_orphan_del(handle, inode);
>  
> -- 
> 2.52.0
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR