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