On Mon, Feb 26, 2024 at 03:13:58PM -0500, Brian Foster wrote:
> On Mon, Feb 26, 2024 at 11:39:21AM -0800, Darrick J. Wong wrote:
> > On Mon, Feb 26, 2024 at 10:40:34AM -0500, Brian Foster wrote:
> > > On Fri, Feb 23, 2024 at 06:05:54PM -0800, Darrick J. Wong wrote:
> > > > On Wed, Feb 14, 2024 at 11:52:31AM -0500, Brian Foster wrote:
> > > > > The background blockgc scanner runs on a 5m interval by default and
> > > > > trims preallocation (post-eof and cow fork) from inodes that are
> > > > > otherwise idle. Idle effectively means that iolock can be acquired
> > > > > without blocking and that the inode has no dirty pagecache or I/O in
> > > > > flight.
> > > > >
> > > > > This simple mechanism and heuristic has worked fairly well for
> > > > > post-eof speculative preallocations. Support for reflink and COW
> > > > > fork preallocations came sometime later and plugged into the same
> > > > > mechanism, with similar heuristics. Some recent testing has shown
> > > > > that COW fork preallocation may be notably more sensitive to blockgc
> > > > > processing than post-eof preallocation, however.
> > > > >
> > > > > For example, consider an 8GB reflinked file with a COW extent size
> > > > > hint of 1MB. A worst case fully randomized overwrite of this file
> > > > > results in ~8k extents of an average size of ~1MB. If the same
> > > > > workload is interrupted a couple times for blockgc processing
> > > > > (assuming the file goes idle), the resulting extent count explodes
> > > > > to over 100k extents with an average size <100kB. This is
> > > > > significantly worse than ideal and essentially defeats the COW
> > > > > extent size hint mechanism.
> > > > >
> > > > > While this particular test is instrumented, it reflects a fairly
> > > > > reasonable pattern in practice where random I/Os might spread out
> > > > > over a large period of time with varying periods of (in)activity.
> > > > > For example, consider a cloned disk image file for a VM or container
> > > > > with long uptime and variable and bursty usage. A background blockgc
> > > > > scan that races and processes the image file when it happens to be
> > > > > clean and idle can have a significant effect on the future
> > > > > fragmentation level of the file, even when still in use.
> > > > >
> > > > > To help combat this, update the heuristic to skip cowblocks inodes
> > > > > that are currently opened for write access during non-sync blockgc
> > > > > scans. This allows COW fork preallocations to persist for as long as
> > > > > possible unless otherwise needed for functional purposes (i.e. a
> > > > > sync scan), the file is idle and closed, or the inode is being
> > > > > evicted from cache.
> > > >
> > > > Hmmm. Thinking this over a bit more, I wonder if we really want this
> > > > heuristic?
> > > >
> > > > If we're doing our periodic background scan then sure, I think it's ok
> > > > to ignore files that are open for write but aren't actively being
> > > > written to.
> > > >
> > > > This might introduce nastier side effects if OTOH we're doing blockgc
> > > > because we've hit ENOSPC and we're trying to free up any blocks that we
> > > > can. I /think/ the way you've written the inode_is_open_for_write check
> > > > means that we scan maximally for ENOSPC.
> > > >
> > >
> > > The intent of the patch was to limit the scope of the heuristic to
> > > the background (non-sync) scan where there are no real guarantees or
> > > predictability. Otherwise I would expect a sync scan to bypass the
> > > heuristic and prioritize the need to free space.
> > >
> > > This is similar to the existing dirty pagecache check for eofblocks
> > > inodes, but I notice that the same check for cowblocks inodes doesn't
> > > seem to care about the type of scan. I suppose one thing to consider for
> > > why that might not matter that much is that IIRC usually this sort of
> > > -ENOSPC handling is preceded by a full fs flush, which probably reduces
> > > the significance of a sync check filter (or lack thereof).
> > >
> > > > However, xfs_blockgc_free_dquots doesn't seem to do synchronous scans
> > > > for EDQUOT. So if we hit quota limits, we won't free maximally, right?
> > > > OTOH I guess we don't really do that now either, so maybe it doesn't
> > > > matter?
> > > >
> > > > <shrug> Thoughts?
> > > >
> > >
> > > Yeah, it seems like it depends on the calling context. I.e.,
> > > xfs_file_buffered_write() -> xfs_blockgc_free_quota() passes the sync
> > > flag for the -EDQUOT case. That case doesn't invoke a flush for -EDQUOT
> > > since it's a a specific quota failure, so ISTM this isn't that much of a
> > > departure from the existing heuristic (which skips cowblocks inodes that
> > > are dirty). Is there a case I'm missing?
> >
> > Not that I can think of. The SYNC/!SYNC decision is entirely based on
> > the caller's state, which (ime) makes me think harder any time I have to
> > reason about the {block,inode}gc function calls.
> >
>
> Not sure what you mean by caller state, but I kind of just view it as a
> poorly named force scan (not that that's a better name). All that really
> matters in this context is the non-force/sync/wait mode that is run by
> the background scanner. Nothing prevents userspace from running a sync
> scan via ioctl() whenever, so it's hard to assume behavior.
>
> Also just a random thought.. you could consider something like a FLUSH
> flag (and/or scan) if you wanted to be more selectively aggressive for
> any of the -EDQUOT handling cases.
>
> > > The question that comes to mind to me is whether those dirty checks in
> > > xfs_prep_free_cowblocks() are more of a correctness thing than a
> > > heuristic..? For example, is that to prevent races between things like
> > > writes allocating some cowblocks and blockgc coming along and removing
> > > them before I/O completes, which actually expects them to exist for
> > > remapping? If so, I suppose that would make me want to tweak the change
> > > a bit to perhaps make the open check first and combine the comments to
> > > better explain what is heuristic and what is rule, but that's only if we
> > > want to keep the patch..
> >
> > The dirty/writeback flag testing in xfs_prep_free_cowblocks exists for
> > correctness -- any time there's live cow staging blocks (as opposed to
> > speculative preallocations) it skips that inode.
> >
>
> Thanks, makes sense. With that, I'd make _prep_free_cowblocks() look
> more something like:
I think we were talking about xfs_prep_free_cowblocks, right?
> /*
> * A cowblocks trim of an inode can have a significant effect on
> * fragmentation even when a reasonable COW extent size hint is set.
> * Therefore, we prefer to not process cowblocks unless they are clean
> * and idle. We can never process a cowblocks inode that is dirty or has
> * in-flight I/O under any circumstances, because outstanding writeback
> * or dio expects targeted COW fork blocks exist through write
> * completion where they can be remapped into the data fork.
> *
> * Therefore, the heuristic used here is to never process inodes
> * currently opened for write from background (i.e. non-sync) scans. For
> * sync scans, use the pagecache/dio state of the inode to ensure we
> * never free COW fork blocks out from under pending I/O.
> */
> if (!sync && inode_is_open_for_write(VFS_I(ip)))
> return false;
> if ((VFS_I(ip)->i_state & I_DIRTY_PAGES) ||
> mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_DIRTY) ||
> mapping_tagged(VFS_I(ip)->i_mapping, PAGECACHE_TAG_WRITEBACK) ||
> atomic_read(&VFS_I(ip)->i_dio_count))
> return false;
>
> ... but again, not clear to me if upstream wants the patch or not. v2 or
> shall I drop it?
I think it's a useful heuristic for background blockgc not to clear
speculative cow preallocations on files that are open for write, even if
there's no contention on the IOLOCK for that file. Maybe it's time to
get that moving again, along with the post-eof block handling revamp?
(Sorry it took me 6 months to reply to this...)
--D
> Brian
>
> > --D
> >
> > > Brian
> > >
> > > > --D
> > > >
> > > > > Suggested-by: Darrick Wong <djwong@xxxxxxxxxx>
> > > > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> > > > > ---
> > > > >
> > > > > This fell out of some of the discussion on a prospective freeze time
> > > > > blockgc scan. I ran this through the same random write test described in
> > > > > that thread and it prevented all cowblocks trimming until the file is
> > > > > released.
> > > > >
> > > > > Brian
> > > > >
> > > > > [1] https://lore.kernel.org/linux-xfs/ZcutUN5B2ZCuJfXr@bfoster/
> > > > >
> > > > > fs/xfs/xfs_icache.c | 20 +++++++++++++++++---
> > > > > 1 file changed, 17 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/fs/xfs/xfs_icache.c b/fs/xfs/xfs_icache.c
> > > > > index dba514a2c84d..d7c54e45043a 100644
> > > > > --- a/fs/xfs/xfs_icache.c
> > > > > +++ b/fs/xfs/xfs_icache.c
> > > > > @@ -1240,8 +1240,13 @@ xfs_inode_clear_eofblocks_tag(
> > > > > */
> > > > > static bool
> > > > > xfs_prep_free_cowblocks(
> > > > > - struct xfs_inode *ip)
> > > > > + struct xfs_inode *ip,
> > > > > + struct xfs_icwalk *icw)
> > > > > {
> > > > > + bool sync;
> > > > > +
> > > > > + sync = icw && (icw->icw_flags & XFS_ICWALK_FLAG_SYNC);
> > > > > +
> > > > > /*
> > > > > * Just clear the tag if we have an empty cow fork or none at all. It's
> > > > > * possible the inode was fully unshared since it was originally tagged.
> > > > > @@ -1262,6 +1267,15 @@ xfs_prep_free_cowblocks(
> > > > > atomic_read(&VFS_I(ip)->i_dio_count))
> > > > > return false;
> > > > >
> > > > > + /*
> > > > > + * A full cowblocks trim of an inode can have a significant effect on
> > > > > + * fragmentation even when a reasonable COW extent size hint is set.
> > > > > + * Skip cowblocks inodes currently open for write on opportunistic
> > > > > + * blockgc scans.
> > > > > + */
> > > > > + if (!sync && inode_is_open_for_write(VFS_I(ip)))
> > > > > + return false;
> > > > > +
> > > > > return true;
> > > > > }
> > > > >
> > > > > @@ -1291,7 +1305,7 @@ xfs_inode_free_cowblocks(
> > > > > if (!xfs_iflags_test(ip, XFS_ICOWBLOCKS))
> > > > > return 0;
> > > > >
> > > > > - if (!xfs_prep_free_cowblocks(ip))
> > > > > + if (!xfs_prep_free_cowblocks(ip, icw))
> > > > > return 0;
> > > > >
> > > > > if (!xfs_icwalk_match(ip, icw))
> > > > > @@ -1320,7 +1334,7 @@ xfs_inode_free_cowblocks(
> > > > > * Check again, nobody else should be able to dirty blocks or change
> > > > > * the reflink iflag now that we have the first two locks held.
> > > > > */
> > > > > - if (xfs_prep_free_cowblocks(ip))
> > > > > + if (xfs_prep_free_cowblocks(ip, icw))
> > > > > ret = xfs_reflink_cancel_cow_range(ip, 0, NULLFILEOFF, false);
> > > > > return ret;
> > > > > }
> > > > > --
> > > > > 2.42.0
> > > > >
> > > >
> > >
> > >
> >
>
