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

* Eric Whitney <enwlinux@gmail.com>:
> * 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.

And of course I botched my search - ETXTBSY, not ETXTBUSY. 50+ instances
remain in the kernel.

So, EOPNOTSUPP or EINVAL would be a clearer indication that the move extents
ioctl is being applied to a file type it can't defrag.  I can go with ETXTBSY
if you really prefer - any of these is better than EBUSY in this case, IMHO.

Eric

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