Re: [PATCH 3/6] xfs: don't stall cowblocks scan if we can't take locks

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jan 18, 2021 at 05:34:12PM +0000, Christoph Hellwig wrote:
> On Thu, Jan 14, 2021 at 01:54:53PM -0800, Darrick J. Wong wrote:
> > On Wed, Jan 13, 2021 at 03:43:57PM +0100, Christoph Hellwig wrote:
> > > On Mon, Jan 11, 2021 at 03:22:52PM -0800, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <djwong@kernel.org>
> > > > 
> > > > Don't stall the cowblocks scan on a locked inode if we possibly can.
> > > > We'd much rather the background scanner keep moving.
> > > 
> > > Wouldn't it make more sense to move the logic to ignore the -EAGAIN
> > > for not-sync calls into xfs_inode_walk_ag?
> > 
> > I'm not sure what you're asking here?  _free_cowblocks only returns
> > EAGAIN for sync calls.  Locking failure for a not-sync call results in a
> > return 0, which means that _walk_ag just moves on to the next inode.
> 
> What I mean is:
> 
>  - always return -EAGAIN when taking the locks fails
>  - don't exit early on -EAGAIN in xfs_inode_walk at least for sync
>    calls, although thinking loud I see no good reason to exit early
>    even for non-sync invocations

Ah, I see, you're asking why don't I make xfs_inode_walk responsible for
deciding what to do about EAGAIN, instead of open-coding that in the
->execute function.  That would be a nice cleanup since the walk
function already has special casing for EFSCORRUPTED.

If I read you correctly, the relevant part of xfs_inode_walk becomes:

	error = execute(batch[i]...);
	xfs_irele(batch[i]);
	if (error == -EAGAIN) {
		if (args->flags & EOF_SYNC)
			skipped++;
		continue;
	}

and the relevant part of xfs_inode_free_eofblocks becomes:

	if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL))
		return -EAGAIN;

I think that would work, and afaict it won't cause any serious problems
with the deferred inactivation series.

--D