On Mon, Mar 02, 2020 at 01:52:52PM -0500, Brian Foster wrote:
> On Mon, Mar 02, 2020 at 06:18:43PM +1100, Dave Chinner wrote:
> > On Thu, Feb 27, 2020 at 08:43:17AM -0500, Brian Foster wrote:
> > > + spin_unlock(&ailp->ail_lock);
> > > +
> > > + xfs_trans_add_item(tp, lip);
> > > + set_bit(XFS_LI_DIRTY, &lip->li_flags);
> > > + tp->t_flags |= XFS_TRANS_DIRTY;
> > > + /* XXX: include ticket owner task fix */
> > > + error = xfs_trans_roll(&tp);
> >
> > So the reservation for this ticket is going to be regranted over and
> > over again and space reserved repeatedly as we work through the
> > locked relog items one at a time?
> >
> > Unfortunately, this violates the rule that prevents rolling
> > transactions from deadlocking. That is, any object that is held
> > locked across the transaction commit and regrant that *might pin the
> > tail of the log* must be relogged in the transaction to move the
> > item forward and prevent it from pinning the tail of the log.
> >
> > IOWs, if one of the items later in the relog list pins the tail of
> > the log we will end up sleeping here:
> >
> > xfs_trans_roll()
> > xfs_trans_reserve
> > xfs_log_regrant
> > xlog_grant_head_check(need_bytes)
> > xlog_grant_head_wait()
> >
> > waiting for the write grant head to move. ANd it never will, because
> > we hold the lock on that item so the AIL can't push it out.
> > IOWs, using a rolling transaction per relog item will not work for
> > processing multiple relog items.
> >
>
> Hm, Ok. I must be missing something about the rolling transaction
> guarantees.
Ok, I know you understand some of the rules, but because I don't
know quite which bit of this complex game you are missing, I'll go
over it from the start (sorry for repeating things you know!).
We're not allowed to hold locked items that are in the AIL over a
transaction reservation call because the transaction reservation may
need to push the log to free up space in the log. Writing back
metadata requires locking the item to flush it, and if we hold it
locked the it can't be flushed. Hence if it pins the tail of the log
and prevents the the reservation from making space available, we
deadlock.
Normally, the only thing that is pinned across a transaction roll is
an inode, and the inode is being logged in every transaction. Hence
it is being continually moved to the head of the log and so can't
pin the tail of the log and prevent the reservation making progress.
The problem here is that having a log reservation for a modification
doesn't guarantee you that log space is immediately available - all
it guarantees you is that the log space will be available if the log
tail if free to move forward.
That's why there are two grant heads. The reservation grant head is
the one that guarantees that you have space available in the log for
the rolling transaction. That is always immediately regranted during
transaction commit, hence we guarantee the rolling transaction will
always fit in the log. The reserve head ensures we never overcommit
the available log space.
The second grant head is the write head, and this tracks the space
immediately available to physically write into the log. This is
essentially tracks the space available for physical writes into the
log. When your write reservation runs out (i.e. after the number of
rolls the log count in the initial transaction reservation
specifies), we have to regrant physical space for the next
transaction in the rolling chain. If the log is physically full, we
have to wait for physical space to be made available.
The only way to increase the amount of physical space available in
the log is to have the tail move forwards. xfs_trans_reserve() does
that by setting a push target for the AIL to flush all the metadata
older than that target. It then blocks waiting for the tail of the
log to move. When the tail of the log moves, the available write
grant space increases because the log head can now physically move
forwards in the log.
Hence when the log is full and we are in a tail pushing situation,
new transactions wait on the reserve grant head to get the log space
guarantee they require. Long duration rolling transactions already
have a log space guarantee from the reserve grant head, so they
end up waiting for the physical log space they require on the write
grant head.
The tail pinning deadlock rolling transactions can trigger is
against the write grant head, not the reserve grant head. If the
tail of the log cannot move, then the write grant space never
increases and xfs_trans_reserve() blocks forever. Hence we cannot
call xfs_trans_roll() whilst holding items locked that have a high
probability of being at the tail of the log.
Given that this relogging functionality is all about preventing
items from either pinning the tail of the log or disappearing off
the tail of the log because they aren't relogged, we have to be very
careful about holding them locked over operations that require
the AIL to be able to make forwards progress....
> > If it's a single transaction, and we join all the locked items
> > for relogging into it in a single transaction commit, then we are
> > fine - we don't try to regrant log space while holding locked items
> > that could pin the tail of the log.
> >
> > We *can* use a rolling transaction if we do this - the AIL has a
> > permenant transaction (plus ticket!) allocated at mount time with
> > a log count of zero, we can then steal reserve/write grant head
> > space into it that ticket at CIL commit time as I mentioned
> > previously. We do a loop like above, but it's basically:
> >
> > {
> > LIST_HEAD(tmp);
> >
> > spin_lock(&ailp->ail_lock);
> > if (list_empty(&ailp->ail_relog_list)) {
> > spin_unlock(&ailp->ail_lock);
> > return;
> > }
> >
> > list_splice_init(&ilp->ail_relog_list, &tmp);
> > spin_unlock(&ailp->ail_lock);
> >
> > xfs_ail_relog_items(ail, &tmp);
> > }
> >
> > This allows the AIL to keep working and building up a new relog
> > as it goes along. hence we can work on our list without interruption
> > or needed to repeatedly take the AIL lock just to get items from the
> > list.
> >
>
> Yeah, I was planning splicing the list as such to avoid cycling the lock
> so much regardless..
>
> > And xfs_ail_relog_items() does something like this:
> >
> > {
> > struct xfs_trans *tp;
> >
> > /*
> > * Make CIL committers trying to change relog status of log
> > * items wait for us to istabilise the relog transaction
> > * again by committing the current relog list and rolling
> > * the transaction.
> > */
> > down_write(&ail->ail_relog_lock);
> > tp = ail->relog_trans;
> >
> > while ((lip = list_first_entry_or_null(&ailp->ail_relog_list,
> > struct xfs_log_item,
> > li_trans)) != NULL) {
> > while (!list_empty(&ailp->ail_relog_list)) {
> > lip = list_first_entry(&ailp->ail_relog_list,
> > struct xfs_log_item, li_trans);
> > list_del_init(&lip->li_trans);
> >
> > xfs_trans_add_item(tp, lip);
> > set_bit(XFS_LI_DIRTY, &lip->li_flags);
> > tp->t_flags |= XFS_TRANS_DIRTY;
> > }
> >
> > error = xfs_trans_roll(&tp);
> > if (error) {
> > SHUTDOWN!
> > }
> > ail->relog_trans = tp;
> > up_write(&ail->ail_relog_lock);
> > }
> >
>
> I think I follow.. The fundamental difference here is basically that we
> commit whatever we locked, right? IOW, the current approach _could_
> technically be corrected, but it would have to lock one item at a time
> (ugh) rather than build up a queue of locked items..?
>
> The reservation stealing approach facilitates this batching because
> instead of only having a guarantee that we can commit one max sized
> relog item at a time, we can commit however many we have queued because
> sufficient reservation has already been acquired.
Yes.
> That does raise another issue in that we presumably want some kind of
> maximum transaction size and/or maximum outstanding relog reservation
> with the above approach. Otherwise it could be possible for a workload
> to go off the rails without any kind of throttling or hueristics
> incorporated in the current (mostly) fixed transaction sizes.
Possibly, though I really don't have any intuition on how big the
relog reservation could possible grow. Right now it doesn't seem
like there's a space problem (single item!), so perhaps this is
something we can defer until we have some further understanding of
how many relog items are active at any given time?
> Perhaps
> it's reasonable enough to cap outstanding relog reservation to the max
> transaction size and return an error to callers that attempt to exceed
Max transaction size the CIL currently uses for large logs is 32MB.
That's an awful lot of relog items....
> it..? It's not clear to me if that would impact the prospective scrub
> use case. Hmmm.. maybe the right thing to do is cap the size of the
> current relog queue so we cap the size of the relog transaction without
> necessarily capping the max outstanding relog reservation. Thoughts on
> that?
Maybe.
Though this seems like an ideal candidate for a relog reservation
grant head. i.e. the transaction reserve structure we pass to
xfs_trans_reserve() has a new field that contains the relog
reservation needed for this transaction, and we use the existing
lockless grant head accounting infrastructure to throttle relogging
to an acceptible bound...
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
