Re: [PATCH 1/2] xfs: trylock underlying buffer on dquot flush

[Brian Foster <bfoster@redhat.com>]

On Tue, Mar 31, 2020 at 11:04:09AM +1100, Dave Chinner wrote:
> On Mon, Mar 30, 2020 at 08:15:44AM -0400, Brian Foster wrote:
> > On Mon, Mar 30, 2020 at 09:46:02AM +1100, Dave Chinner wrote:
> > > On Thu, Mar 26, 2020 at 09:17:02AM -0400, Brian Foster wrote:
> > > > A dquot flush currently blocks on the buffer lock for the underlying
> > > > dquot buffer. In turn, this causes xfsaild to block rather than
> > > > continue processing other items in the meantime. Update
> > > > xfs_qm_dqflush() to trylock the buffer, similar to how inode buffers
> > > > are handled, and return -EAGAIN if the lock fails. Fix up any
> > > > callers that don't currently handle the error properly.
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/xfs_dquot.c      |  6 +++---
> > > >  fs/xfs/xfs_dquot_item.c |  3 ++-
> > > >  fs/xfs/xfs_qm.c         | 14 +++++++++-----
> > > >  3 files changed, 14 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/fs/xfs/xfs_dquot.c b/fs/xfs/xfs_dquot.c
> > > > index 711376ca269f..af2c8e5ceea0 100644
> > > > --- a/fs/xfs/xfs_dquot.c
> > > > +++ b/fs/xfs/xfs_dquot.c
> > > > @@ -1105,8 +1105,8 @@ xfs_qm_dqflush(
> > > >  	 * Get the buffer containing the on-disk dquot
> > > >  	 */
> > > >  	error = xfs_trans_read_buf(mp, NULL, mp->m_ddev_targp, dqp->q_blkno,
> > > > -				   mp->m_quotainfo->qi_dqchunklen, 0, &bp,
> > > > -				   &xfs_dquot_buf_ops);
> > > > +				   mp->m_quotainfo->qi_dqchunklen, XBF_TRYLOCK,
> > > > +				   &bp, &xfs_dquot_buf_ops);
> > > >  	if (error)
> > > >  		goto out_unlock;
> > > >  
> > > > @@ -1177,7 +1177,7 @@ xfs_qm_dqflush(
> > > >  
> > > >  out_unlock:
> > > >  	xfs_dqfunlock(dqp);
> > > > -	return -EIO;
> > > > +	return error;
> > > >  }
> > > >  
> > > >  /*
> > > > diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> > > > index cf65e2e43c6e..baad1748d0d1 100644
> > > > --- a/fs/xfs/xfs_dquot_item.c
> > > > +++ b/fs/xfs/xfs_dquot_item.c
> > > > @@ -189,7 +189,8 @@ xfs_qm_dquot_logitem_push(
> > > >  		if (!xfs_buf_delwri_queue(bp, buffer_list))
> > > >  			rval = XFS_ITEM_FLUSHING;
> > > >  		xfs_buf_relse(bp);
> > > > -	}
> > > > +	} else if (error == -EAGAIN)
> > > > +		rval = XFS_ITEM_LOCKED;
> > > 
> > > Doesn't xfs_inode_item_push() also have this problem in that it
> > > doesn't handle -EAGAIN properly?
> > > 
> > > Also, we can get -EIO, -EFSCORRUPTED, etc here. They probably
> > > shouldn't return XFS_ITEM_SUCCESS, either....
> > > 
> > 
> > Good point. I'm actually not sure what we should return in that case
> > given the item return codes all seem to assume a valid state. We could
> > define an XFS_ITEM_ERROR return, but I'm not sure it's worth it for what
> > is currently stat/tracepoint logic in the caller.  Perhaps a broader
> > rework of error handling in this context is in order that would lift
> > generic (fatal) error handling into xfsaild.
> 
> Yeah, that's where my thoughts were heading as well.
> 
> > E.g., I see that
> > xfs_qm_dqflush() is inconsistent by itself in that the item is removed
> > from the AIL if we're already shut down, but not if that function
> > invokes the shutdown; we shutdown if the direct xfs_dqblk_verify() call
> > fails but not if the read verifier (which also looks like it calls
> > xfs_dqblk_verify() on every on-disk dquot) returns -EFSCORRUPTED, etc.
> > It might make some sense to let iop_push() return negative error codes
> > if that facilitates consistent error handling...
> 
> Yes, it's a bit of a mess. I suspect that what we should be doing
> here is pulling the failed buffer write retry code up into the main
> push loop. That is, we can set LI_FAILED on log items that fail to
> flush, either directly at submit time, or at IO completion for write
> errors.
> 
> Then we can have the main AIL loop set LI_FAILED on push failures,
> and also the main loop detect LI_FAILED directly and call a new
> ->iop_resubmit() function rather than having to handle that the
> resubmit cases as special cases in every ->iop_push() path.
> 

I'm not sure we want to use LI_FAILED in failure to flush (i.e. push
failure) situations because it's currently used specifically to indicate
that a particular item requires resubmit when it already has been
successfully flushed. This avoids the need for a post I/O error push to
retry an already locked flush lock (and flush attempt) and subsequently
cause the item to remain stuck on the AIL. It still might make sense to
refactor the existing LI_FAILED implementation into ->iop_resubmit()
callbacks for those items that use it, though.

That also doesn't preclude refactoring some sort of generic push failure
error handling into xfsaild for the sake of consistency. It's just not
immediately clear to me what it should look like. Perhaps I'll poke at
it a bit once I get the next rfc of the relog work settled and posted
(soon)..

Brian

> That seems like a much cleaner way of handling submission failure
> and retries for all log item types that need it compared to the way
> we currently handle it for buffers...
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>