Re: [PATCH 4/4] xfs: drop freeze protection when running GETFSMAP

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

On Mon, Mar 08, 2021 at 10:05:55AM +1100, Dave Chinner wrote:
> On Sun, Mar 07, 2021 at 12:26:02PM -0800, Darrick J. Wong wrote:
> > From: Darrick J. Wong <djwong@kernel.org>
> > 
> > A recent log refactoring patchset from Brian Foster relaxed fsfreeze
> > behavior with regards to the buffer cache -- now freeze only waits for
> > pending buffer IO to finish, and does not try to drain the buffer cache
> > LRU.  As a result, fsfreeze should no longer stall indefinitely while
> > fsmap runs.  Drop the sb_start_write calls around fsmap invocations.
> > 
> > Signed-off-by: Darrick J. Wong <djwong@kernel.org>
> > ---
> >  fs/xfs/xfs_fsmap.c |   14 +++++---------
> >  1 file changed, 5 insertions(+), 9 deletions(-)
> > 
> > 
> > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > index 9ce5e7d5bf8f..34f2b971ce43 100644
> > --- a/fs/xfs/xfs_fsmap.c
> > +++ b/fs/xfs/xfs_fsmap.c
> > @@ -904,14 +904,6 @@ xfs_getfsmap(
> >  	info.fsmap_recs = fsmap_recs;
> >  	info.head = head;
> >  
> > -	/*
> > -	 * If fsmap runs concurrently with a scrub, the freeze can be delayed
> > -	 * indefinitely as we walk the rmapbt and iterate over metadata
> > -	 * buffers.  Freeze quiesces the log (which waits for the buffer LRU to
> > -	 * be emptied) and that won't happen while we're reading buffers.
> > -	 */
> > -	sb_start_write(mp->m_super);
> > -
> >  	/* For each device we support... */
> >  	for (i = 0; i < XFS_GETFSMAP_DEVS; i++) {
> >  		/* Is this device within the range the user asked for? */
> > @@ -934,6 +926,11 @@ xfs_getfsmap(
> >  		if (handlers[i].dev > head->fmh_keys[0].fmr_device)
> >  			memset(&dkeys[0], 0, sizeof(struct xfs_fsmap));
> >  
> > +		/*
> > +		 * Grab an empty transaction so that we can use its recursive
> > +		 * buffer locking abilities to detect cycles in the rmapbt
> > +		 * without deadlocking.
> > +		 */
> >  		error = xfs_trans_alloc_empty(mp, &tp);
> >  		if (error)
> >  			break;
> 
> Took me a moment to work out that this is just adding a comment
> because it wasn't mentioned in the commit log. Somewhat unrelated to
> the bug fix but it's harmless so I don't see any need for you to
> do any extra work to respin this patch to remove it.

I'll add a sentence to the commit message explaining why we're adding a
seemingly random (but related!) comment:

"While we're cleaning things, add a comment to the xfs_trans_alloc_empty
call explaining why we're running around with empty transactions."

--D

> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com