| Message ID | 161040737263.1582114.4973977520111925461.stgit@magnolia (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xfs: try harder to reclaim space when we run out | expand |
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?
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. --D
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
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
On Mon, Jan 18, 2021 at 11:37:18AM -0800, Darrick J. Wong wrote: > 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. Exactly!
On Mon, Jan 18, 2021 at 07:39:58PM +0000, Christoph Hellwig wrote: > On Mon, Jan 18, 2021 at 11:37:18AM -0800, Darrick J. Wong wrote: > > 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. > > Exactly! D'oh. I tried making that change, but ran into the problem that *args isn't necessarily an eofb structure, and xfs_qm_dqrele_all_inodes passes a uint pointer. I could define a new XFS_INODE_WALK_SYNC flag and update the blockgc callers to set that if EOF_FLAGS_SYNC is set... --D
On Mon, Jan 18, 2021 at 11:44:22AM -0800, Darrick J. Wong wrote: > D'oh. I tried making that change, but ran into the problem that *args > isn't necessarily an eofb structure, and xfs_qm_dqrele_all_inodes passes > a uint pointer. I could define a new XFS_INODE_WALK_SYNC flag and > update the blockgc callers to set that if EOF_FLAGS_SYNC is set... That does actually sound cleaner to me, but I don't want to burden that work on you. Feel free to stick to the current version and we can clean this up later.
diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c index 66af84c578b5..4e226827b33d 100644 --- a/fs/xfs/xfs_icache.c +++ b/fs/xfs/xfs_icache.c @@ -1624,17 +1624,31 @@ xfs_inode_free_cowblocks( void *args) { struct xfs_eofblocks *eofb = args; + bool wait; int ret = 0; + wait = eofb && (eofb->eof_flags & XFS_EOF_FLAGS_SYNC); + if (!xfs_prep_free_cowblocks(ip)) return 0; if (!xfs_inode_matches_eofb(ip, eofb)) return 0; - /* Free the CoW blocks */ - xfs_ilock(ip, XFS_IOLOCK_EXCL); - xfs_ilock(ip, XFS_MMAPLOCK_EXCL); + /* + * If the caller is waiting, return -EAGAIN to keep the background + * scanner moving and revisit the inode in a subsequent pass. + */ + if (!xfs_ilock_nowait(ip, XFS_IOLOCK_EXCL)) { + if (wait) + return -EAGAIN; + return 0; + } + if (!xfs_ilock_nowait(ip, XFS_MMAPLOCK_EXCL)) { + if (wait) + ret = -EAGAIN; + goto out_iolock; + } /* * Check again, nobody else should be able to dirty blocks or change @@ -1644,6 +1658,7 @@ xfs_inode_free_cowblocks( ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false); xfs_iunlock(ip, XFS_MMAPLOCK_EXCL); +out_iolock: xfs_iunlock(ip, XFS_IOLOCK_EXCL); return ret;