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:   Tue, 19 Jul 2022 17:35:12 -0400
From:   Eric Whitney <enwlinux@gmail.com>
To:     Jan Kara <jack@suse.cz>
Cc:     Eric Whitney <enwlinux@gmail.com>, linux-ext4@vger.kernel.org,
        tytso@mit.edu
Subject: Re: [PATCH] ext4: minor defrag code improvements
Message-ID: <YtcjkMcYQCATlt0Y@debian-BULLSEYE-live-builder-AMD64>
References: <20220621143340.2268087-1-enwlinux@gmail.com>
 <20220714115326.qhjsrchoepnnsffu@quack3>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220714115326.qhjsrchoepnnsffu@quack3>
Precedence: bulk

* Jan Kara <jack@suse.cz>:
> 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

Hi Jan - thanks for your review.

I think EOPNOTSUPP is better than EBUSY when EXT4_IOC_MOVE_EXT is applied to
a swap file because it's a much more direct message indicating that ext4
doesn't support swap file defragmentation.  With the exception of the two
sites modified by this patch, all the other sites in the defrag code where
checks are made for unsupported file types or file system configurations,
EOPNOTSUPP is returned.  Because EBUSY really doesn't seem to convey what's
happening here very well - this isn't so much a case of a potentially
temporarily busy resource as an attempt to perform an operation that will
never succeed - another choice seemed more appropriate.  I picked EOPNOTSUPP
simply because it's used in those other sites in the defrag code, and was
trying to be consistent. (The defrag code reports EBUSYs when it can't
release pages with references on them.)

EINVAL could be an alternative.  It's not quite as direct but it does
convey the idea that an argument to the ioctl is wrong.  The defrag code
uses it (a little inconsistently) when argument values are out of range or
otherwise invalid.

Is ETXTBUSY still reported by the kernel?  I couldn't find it in a search after
reading this:  lwn.net/Articles/866493/
I didn't consider that because an executable wasn't involved - interesting that
it was used for some operations applied to swap files.

At any rate, I'm open to suggestions here - just trying to clean up a few
things after a little code review.

Thanks,
Eric

> 
> > ---
> >  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