Re: [PATCH] xfs: hold xfs_buf locked between shortform->leaf conversion and the addition of an attribute

[Dave Chinner <david@fromorbit.com>]

On Thu, Aug 10, 2017 at 10:55:48AM -0700, Darrick J. Wong wrote:
> On Thu, Aug 10, 2017 at 10:52:49AM -0400, Brian Foster wrote:
> > On Thu, Aug 10, 2017 at 03:09:09PM +0300, Alex Lyakas wrote:
> > > Hi Dave,
> > > 
> > > Thanks for the explanation. So it seems we cannot move forward with this
> > > fix.
> > > 
> > 
> > I don't think this completely invalidates the fix.. Dave is pointing out
> > a flaw that the deferred ops infrastructure doesn't properly handle the
> > technique we want to use here. IOW, it means there's a dependency that
> > needs to be implemented first.
> > 
> > FWIW, I also think this means that your approach on the older kernel to
> > join/hold the buffer to the finished transaction may be the right
> > approach (depending on whether I follow the perm transaction code
> > correctly or not, see below), but I think you'd need to relog the buffer
> > as well.

Yes, the problem exists in 3.18 via the roll in xfs_bmap_finish()
so it would also need to be done there, too.

> > 
> > > Will somebody else in XFS community be working on fixing this issue? As you
> > > pointed out, it exists for over two decades. Our production systems hit this
> > > every couple of days, and shutting down the filesystem causes outage.
> > > 
> > 
> > I'm guessing the defer infrastructure needs to handle relogging a buffer
> > similar to how it currently handles joining/relogging inodes..?

Yup, pretty much identical, and only a 10-20 lines of new code, I
think.

> > > The problem is that the locked buffer is not joined and logged in
> > > the rolling transactions run in xfs_defer_ops. Hence it can pin the
> > > tail of the AIL, and this can prevent the transaction roll from
> > > regranting the log space necessary to continue rolling the
> > > transaction for the required number of transactions to complete the
> > > deferred ops. If this happens, we end up with a log space deadlock.
> > > 
> > > Hence if we are holding an item that we logged in a transaction
> > > locked and we roll the transaction, we have to join, hold and log it
> > > in each subsequent transaction until we have finished with the item
> > > and can unlock and release it.
> > > 
> > > This is documented in xfs_trans_roll():
> > > 
> > >        /*
> > >         * Reserve space in the log for th next transaction.
> > >         * This also pushes items in the "AIL", the list of logged items,
> > >         * out to disk if they are taking up space at the tail of the log
> > >         * that we want to use.  This requires that either nothing be locked
> > >         * across this call, or that anything that is locked be logged in
> > >         * the prior and the next transactions.
> > >         */
> > > 
> > 
> > Good catch, though I'm wondering whether it's a real enough problem atm
> > to block this fix. A few thoughts/questions:
> > 
> > 1.) The transaction in this case has a t_log_count of 3, presumably to
> > cover the commits by the historical bmap_finish, the trans roll and the
> > final commit. If I'm following the permanent transaction code correctly,
> > doesn't that mean that we have room for at least 2 rolls (and 3 commits)
> > before this task would actually block on log reservation? AFAICT it
> > looks like the commit would dec ticket->t_cnt and replenish the current
> > log reservation. The subsequent xfs_trans_reserve() would just return if
> > t_cnt > 0.
> > 
> > This of course doesn't accommodate the fact the xfs_defer_finish() can
> > now roll a transaction an indeterminate number of times, which probably
> > needs to be handled in general, but...
> 
> I'd been wondering if tr_logcount needed upward adjusting, but so far
> haven't observed any problems.

That won't avoid the general problem, though, just increase log
reservation pressure from active transactions.

> > 2.) It doesn't look like we actually defer any ops in this situation
> > unless rmapbt is enabled, assuming that we limit holding the buffer to
> > the empty leaf case, at least (see my comment on the previous version).
> > I also don't see where a deferred rmapbt update would itself ever roll
> > the transaction.
> 
> rmapbt split causes the agfl to hit the low water mark and refresh,
> requiring an allocation ... but I think that's all stuffed in the same
> transaction.  (So yeah I think I agree with you)

I haven't looked that far, but I'd prefer we fix the problem now
while we are looking at it because it doesn't seem that hard to
fix...

> > 3.) The buffer in this case is a new allocation, which I think means the
> > risk of it pinning the tail and causing a log deadlock here means that
> > on top of somehow depleting the initial permanent reservation, we'd have
> > to exhaust the log in the time between the first commit and the last
> > reservation.
> > 
> > Given the above, it seems reasonably safe enough to me to merge this
> > change as is and fix up the deferred ops stuff after the fact
> > (considering we know we need to rework the xattr stuff as such anyways).
> > This is still a landmine that should be fixed up, but I wouldn't be
> > against an ASSERT() or something for the time being if we could somehow
> > verify that the transaction ticket didn't require any extra reservation.
> > 
> > OTOH, just adding deferred ops buffer relogging might not be too much
> > trouble either. ;) Anyways, thoughts?
> 
> I don't think it'd be difficult to add a _defer_bjoin operation that
> maintains a list of buffers that we need to bhold across rolls.

Just a small array like inodes currently use would be sufficient.
We only need to hold one buffer right now....

> I think xfs_buf->b_list is only used for delwri buffers, and a buffer
> cannot be part of a transaction /and/ on a delwri list at the same time,
> right?  So it shouldn't be hard to whip something up and couple this
> patch to that.

Reading xfs_buf_item_push() answers that question:

        if (!xfs_buf_delwri_queue(bp, buffer_list))
                rval = XFS_ITEM_FLUSHING;
        xfs_buf_unlock(bp);
        return rval;

So, yes, a buffer can be on the delwri queue and be part of a
transaction at the same time because the buffers on the delwri queue
get unlocked once they are queued. If a transaction locks and joins
the buffer while it is on the delwri queue, the commit will pin the
buffer in memory before unlocking it and
xfs_buf_delwri_submit_nowait() will see it pinned and skip over it.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com