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:

quoted

On Wed, Jan 13, 2021 at 03:43:57PM +0100, Christoph Hellwig wrote:

quoted

On Mon, Jan 11, 2021 at 03:22:52PM -0800, Darrick J. Wong wrote:

quoted

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