Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 14 Jul 2022 13:53:26 +0200
From:   Jan Kara <jack@suse.cz>
To:     Eric Whitney <enwlinux@gmail.com>
Cc:     linux-ext4@vger.kernel.org, tytso@mit.edu
Subject: Re: [PATCH] ext4: minor defrag code improvements
Message-ID: <20220714115326.qhjsrchoepnnsffu@quack3>
References: <20220621143340.2268087-1-enwlinux@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220621143340.2268087-1-enwlinux@gmail.com>
Precedence: bulk

On Tue 21-06-22 10:33:40, Eric Whitney wrote:
> Modify two error paths returning EBUSY for bad argument file types to
> return EOPNOTSUPP instead.  Move an extent tree search whose results are
> only occasionally required to the site always requiring them for
> improved efficiency.  Address a few typos.
> 
> Signed-off-by: Eric Whitney <enwlinux@gmail.com>

So why is EOPNOTSUPP better than EBUSY? Honestly we are rather inconsistent
with errors returned for various operations on swapfile -
read/write/fallocate/truncate return ETXTBSY, unlink returns EPERM, some
ext4 ioctls return EINVAL... I guess ETXTBSY is the most common return
value?

Otherwise the patch looks good.

								Honza

> ---
>  fs/ext4/move_extent.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/fs/ext4/move_extent.c b/fs/ext4/move_extent.c
> index 701f1d6a217f..4e4b0452106e 100644
> --- a/fs/ext4/move_extent.c
> +++ b/fs/ext4/move_extent.c
> @@ -472,19 +472,17 @@ mext_check_arguments(struct inode *orig_inode,
>  	if (IS_IMMUTABLE(donor_inode) || IS_APPEND(donor_inode))
>  		return -EPERM;
>  
> -	/* Ext4 move extent does not support swapfile */
> +	/* Ext4 move extent does not support swap files */
>  	if (IS_SWAPFILE(orig_inode) || IS_SWAPFILE(donor_inode)) {
> -		ext4_debug("ext4 move extent: The argument files should "
> -			"not be swapfile [ino:orig %lu, donor %lu]\n",
> +		ext4_debug("ext4 move extent: The argument files should not be swap files [ino:orig %lu, donor %lu]\n",
>  			orig_inode->i_ino, donor_inode->i_ino);
> -		return -EBUSY;
> +		return -EOPNOTSUPP;
>  	}
>  
>  	if (ext4_is_quota_file(orig_inode) && ext4_is_quota_file(donor_inode)) {
> -		ext4_debug("ext4 move extent: The argument files should "
> -			"not be quota files [ino:orig %lu, donor %lu]\n",
> +		ext4_debug("ext4 move extent: The argument files should not be quota files [ino:orig %lu, donor %lu]\n",
>  			orig_inode->i_ino, donor_inode->i_ino);
> -		return -EBUSY;
> +		return -EOPNOTSUPP;
>  	}
>  
>  	/* Ext4 move extent supports only extent based file */
> @@ -631,11 +629,11 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  		if (ret)
>  			goto out;
>  		ex = path[path->p_depth].p_ext;
> -		next_blk = ext4_ext_next_allocated_block(path);
>  		cur_blk = le32_to_cpu(ex->ee_block);
>  		cur_len = ext4_ext_get_actual_len(ex);
>  		/* Check hole before the start pos */
>  		if (cur_blk + cur_len - 1 < o_start) {
> +			next_blk = ext4_ext_next_allocated_block(path);
>  			if (next_blk == EXT_MAX_BLOCKS) {
>  				ret = -ENODATA;
>  				goto out;
> @@ -663,7 +661,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp, __u64 orig_blk,
>  		donor_page_index = d_start >> (PAGE_SHIFT -
>  					       donor_inode->i_blkbits);
>  		offset_in_page = o_start % blocks_per_page;
> -		if (cur_len > blocks_per_page- offset_in_page)
> +		if (cur_len > blocks_per_page - offset_in_page)
>  			cur_len = blocks_per_page - offset_in_page;
>  		/*
>  		 * Up semaphore to avoid following problems:
> -- 
> 2.30.2
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR