Re: [PATCH v2] btrfs: add goto in btrfs_defrag_file for error handling

[Boris Burkov <boris@bur.io>]

On Wed, May 05, 2021 at 09:26:28AM +0800, Tian Tao wrote:
> ret is assigned -EAGAIN at line 1455 and then reassigned defrag_count
> at line 1547 after exiting the while loop.this causes the
> btrfs_defrag_file function to not return the correct value in the event
> of an error, this patch fixed this issue.

This looks like a correct fix, in that it locally improves what it
claims to improve. However, I have some questions about the style and
consistency of the function as a whole as a result. I think Dave had
a similar comment in his very first reply on v1.

The loop has the following early exit points:
fs unmounted
cancellation
swapfile/error in cluster_pages_for_defrag
newer_off == (u64)-1
error (ENOMEM or ENOENT) in find_new_extents

To me, it is confusing that of all these, only cancellation goes to a
label called "error". I would expect at least the swapfile/cluster case
to also jump to error. find_new_extents is interesting, because ENOENT
could be semantically special as an error and warrant a break rather
than a goto error, so we ought to figure that out correctly too.

If there is some good reason that only cancellation should receive this
treatment, and that some early exit cases should break or goto out_ra
then I would at least name the new label "cancel" and write a comment or
a note in the git commit explaining the difference.

Thinking out loud, I suspect a way to really fix this messy function is
to do something like lift the contents of the while loop into a helper
function which returns the next incremental defrag_count, an error, or 0
for done.

Thanks for the fix,
Boris

> 
> Signed-off-by: Tian Tao <tiantao6@hisilicon.com>
> ---
> 
> v2: rewrite the patch, patch name and commit message.
> ---
>  fs/btrfs/ioctl.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index ee1dbab..5713450 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -1453,7 +1453,7 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
>  		if (btrfs_defrag_cancelled(fs_info)) {
>  			btrfs_debug(fs_info, "defrag_file cancelled");
>  			ret = -EAGAIN;
> -			break;
> +			goto error;
>  		}
>  
>  		if (!should_defrag_range(inode, (u64)i << PAGE_SHIFT,
> @@ -1531,6 +1531,8 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
>  		}
>  	}
>  
> +	ret = defrag_count;
> +error:
>  	if ((range->flags & BTRFS_DEFRAG_RANGE_START_IO)) {
>  		filemap_flush(inode->i_mapping);
>  		if (test_bit(BTRFS_INODE_HAS_ASYNC_EXTENT,
> @@ -1544,8 +1546,6 @@ int btrfs_defrag_file(struct inode *inode, struct file *file,
>  		btrfs_set_fs_incompat(fs_info, COMPRESS_ZSTD);
>  	}
>  
> -	ret = defrag_count;
> -
>  out_ra:
>  	if (do_compress) {
>  		btrfs_inode_lock(inode, 0);
> -- 
> 2.7.4
>