Re: [PATCH v2] ext4: reset retry counter when ext4_alloc_file_blocks() makes progress

[Jan Kara <jack@suse.cz>]

On Fri 19-02-21 12:25:19, Eric Whitney wrote:
> Change the retry policy in ext4_alloc_file_blocks() to allow for a full
> retry cycle whenever a portion of an allocation request has been
> fulfilled.  A large allocation request often results in multiple calls
> to ext4_map_blocks(), each of which is potentially subject to a
> temporary ENOSPC condition and retry cycle.  The current code only
> allows for a single retry cycle.
> 
> This patch does not address a known bug or reported complaint.
> However, it should make block allocation for fallocate and zero range
> more robust.
> 
> In addition, simplify the conditional controlling the allocation while
> loop, where testing len alone is sufficient.  Remove the assignment to
> ret2 in the error path after the call to ext4_map_blocks() since its
> value isn't subsequently used.
> 
> v2: Silence smatch warning by initializing ret.
> 
> For smatch warning:
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

Looks good to me. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza
 

> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>
> ---
>  fs/ext4/extents.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 3960b7ec3ab7..77c84d6f1af6 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -4382,8 +4382,7 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset,
>  {
>  	struct inode *inode = file_inode(file);
>  	handle_t *handle;
> -	int ret = 0;
> -	int ret2 = 0, ret3 = 0;
> +	int ret = 0, ret2 = 0, ret3 = 0;
>  	int retries = 0;
>  	int depth = 0;
>  	struct ext4_map_blocks map;
> @@ -4408,7 +4407,7 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset,
>  	depth = ext_depth(inode);
>  
>  retry:
> -	while (ret >= 0 && len) {
> +	while (len) {
>  		/*
>  		 * Recalculate credits when extent tree depth changes.
>  		 */
> @@ -4430,9 +4429,13 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset,
>  				   inode->i_ino, map.m_lblk,
>  				   map.m_len, ret);
>  			ext4_mark_inode_dirty(handle, inode);
> -			ret2 = ext4_journal_stop(handle);
> +			ext4_journal_stop(handle);
>  			break;
>  		}
> +		/*
> +		 * allow a full retry cycle for any remaining allocations
> +		 */
> +		retries = 0;
>  		map.m_lblk += ret;
>  		map.m_len = len = len - ret;
>  		epos = (loff_t)map.m_lblk << inode->i_blkbits;
> @@ -4450,11 +4453,8 @@ static int ext4_alloc_file_blocks(struct file *file, ext4_lblk_t offset,
>  		if (unlikely(ret2))
>  			break;
>  	}
> -	if (ret == -ENOSPC &&
> -			ext4_should_retry_alloc(inode->i_sb, &retries)) {
> -		ret = 0;
> +	if (ret == -ENOSPC && ext4_should_retry_alloc(inode->i_sb, &retries))
>  		goto retry;
> -	}
>  
>  	return ret > 0 ? ret2 : ret;
>  }
> -- 
> 2.20.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR