On Tue, Aug 06, 2019 at 08:53:21AM -0400, Brian Foster wrote: > On Tue, Aug 06, 2019 at 03:33:38PM +1000, Dave Chinner wrote: > > On Tue, Aug 06, 2019 at 09:28:26AM +1000, Dave Chinner wrote: > > > On Mon, Aug 05, 2019 at 01:53:26PM -0400, Brian Foster wrote: > > > > On Thu, Aug 01, 2019 at 12:17:42PM +1000, Dave Chinner wrote: > > > > > From: Dave Chinner <dchinner@xxxxxxxxxx> > > > > > > > > > > We currently wake anything waiting on the log tail to move whenever > > > > > the log item at the tail of the log is removed. Historically this > > > > > was fine behaviour because there were very few items at any given > > > > > LSN. But with delayed logging, there may be thousands of items at > > > > > any given LSN, and we can't move the tail until they are all gone. > > > > > > > > > > Hence if we are removing them in near tail-first order, we might be > > > > > waking up processes waiting on the tail LSN to change (e.g. log > > > > > space waiters) repeatedly without them being able to make progress. > > > > > This also occurs with the new sync push waiters, and can result in > > > > > thousands of spurious wakeups every second when under heavy direct > > > > > reclaim pressure. > > > > > > > > > > To fix this, check that the tail LSN has actually changed on the > > > > > AIL before triggering wakeups. This will reduce the number of > > > > > spurious wakeups when doing bulk AIL removal and make this code much > > > > > more efficient. > > > > > > > > > > XXX: occasionally get a temporary hang in xfs_ail_push_sync() with > > > > > this change - log force from log worker gets things moving again. > > > > > Only happens under extreme memory pressure - possibly push racing > > > > > with a tail update on an empty log. Needs further investigation. > > > > > > > > > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> > > > > > --- > > > > > > > > Ok, this addresses the wakeup granularity issue mentioned in the > > > > previous patch. Note that I was kind of wondering why we wouldn't base > > > > this on the l_tail_lsn update in xlog_assign_tail_lsn_locked() as > > > > opposed to the current approach. > > > > > > Because I didn't think of it? :) > > > > > > There's so much other stuff in this patch set I didn't spend a > > > lot of time thinking about other alternatives. this was a simple > > > code transformation that did what I wanted, and I went on to burning > > > brain cells on other more complex issues that needs to be solved... > > > > > > > For example, xlog_assign_tail_lsn_locked() could simply check the > > > > current min item against the current l_tail_lsn before it does the > > > > assignment and use that to trigger tail change events. If we wanted to > > > > also filter out the other wakeups (as this patch does) then we could > > > > just pass a bool pointer or something that returns whether the tail > > > > actually changed. > > > > > > Yeah, I'll have a look at this - I might rework it as additional > > > patches now the code is looking at decisions based on LSN rather > > > than if the tail log item changed... > > > > Ok, this is not worth the complexity. The wakeup code has to be able > > to tell the difference between a changed tail lsn and an empty AIL > > so that wakeups can be issued when the AIL is finally emptied. > > Unmount (xfs_ail_push_all_sync()) relies on this, and > > xlog_assign_tail_lsn_locked() hides the empty AIL from the caller > > by returning log->l_last_sync_lsn to the caller. > > > > Wouldn't either case just be a wakeup from xlog_assign_tail_lsn_locked() > (which should probably be renamed if we took that approach)? It's called > when we've removed the min item from the AIL and so potentially need to > update the tail lsn. Not easily, because xlog_assign_tail_lsn_locked() is also used to grab the current tail when we are formatting the log header during a CIL checkpoint. We do not want to be doing wakeups there. And, to tell the truth, I don't really want to screw with a function that provides on-disk information for log recovery in this series. That brings a whole new level of jeopardy to this patch set I'd prefer to avoid.... > > Hence the wakeup code still has to check for an empty AIL if the > > tail has changed if we use the return value of > > xlog_assign_tail_lsn_locked() as the tail LSN. At which point, the > > logic becomes somewhat convoluted, and it's far simpler to use > > __xfs_ail_min_lsn as it returns when the log is empty. > > > > So, nice idea, but it doesn't make the code simpler or easier to > > understand.... > > It's not that big of a deal either way. BTW on another quick look, I > think something like xfs_ail_update_tail(ailp, old_tail) is a bit more > self-documenting that xfs_ail_delete_finish(ailp, old_lsn). I had already renamed it to xfs_ail_update_finish() when I updated the last patch to include the bulk update case. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx
