On Wed, Mar 03, 2021 at 12:23:04PM +1100, Dave Chinner wrote: > On Mon, Mar 01, 2021 at 08:32:18AM -0500, Brian Foster wrote: > > On Mon, Mar 01, 2021 at 03:54:22PM +1100, Dave Chinner wrote: > > > On Sat, Feb 27, 2021 at 11:25:06AM -0500, Brian Foster wrote: > > > > On Fri, Feb 26, 2021 at 09:03:05AM +1100, Dave Chinner wrote: > > > > > This is really nasty behaviour, and it's only recently that I've got > > > > > a handle on it. I found it because my original "async CIL push" code > > > > > resulted in long stalls every time the log is filled and the tail is > > > > > pinned by a buffer that is being relogged in this manner.... > > > > > > > > > > I'm not sure how to fix this yet - the AIL needs to block the front > > > > > end relogging to allow the buffer to be unpinned. Essentially, we > > > > > need to hold the pinned items locked across a CIL push to guarantee > > > > > they are unpinned, but that's the complete opposite of what the AIL > > > > > currently does to prevent the front end from seeing long tail lock > > > > > latencies when modifying stuff.... > > > > > > > > When this stall problem manifests, I'm assuming it's exacerbated by > > > > delayed logging and the commit record behavior I described above. If > > > > that's the case, could the AIL communicate writeback pressure through > > > > affected log items such that checkpoints in which they are resident are > > > > flushed out completely/immediately when the checkpoints occur? I suppose > > > > that would require a log item flag or some such, which does raise a > > > > concern of unnecessarily tagging many items (it's not clear to me how > > > > likely that really is), but I'm curious if that would be an effective > > > > POC at least.. > > > > > > I don't think we need to do anything like that. All we need to do to > > > ensure that the AIL can flush a pinned buffer is to lock it, kick > > > the log and wait for the pin count to go to zero. Then we can write > > > it just fine, blocking only the front end transactions that need > > > that buffer lock. Same goes for inodes, though xfs_iunpin_wait() > > > already does this.... > > > > > > > Yeah, but why would we want to block xfsaild on a single item like that? > > Who said anything about blocking the AIL on a single item? :) > > > Wouldn't holding the item locked like that just create a new stall point > > within xfsaild? Maybe I'm missing something, but what you describe here > > basically just sounds like a "lock the item and run a sync log force" > > pattern. > > What I was thinking is that if the item is pinned and at the > tail of the log, then we leave it locked when we flush it rather > than unlocking it and relocking it when the delwri submit code gets > to it. If it gets unpinned before the delwri code gets to it, all > good. If not, the delwri code being unable to flush it will feed > back up into the AIL to trigger a log force, which will unpin it > in the near future and it will be written on the next AIL delwri > submit cycle. > I'm not sure what you mean by leaving the item locked when we flush it if it is pinned, since we don't flush pinned items. Perhaps implied is that this would trylock the buffer first, then only worry about if it's pinned if we acquire the lock. If so (and the pin is at the tail), hold the lock and kick the log as a means to ensure that xfsaild is guaranteed next access to the buffer once unpinned. Hm? IIUC, that seems interesting. Though as noted in the flush optimization series, I am a little leery about issuing (even async) log forces with locks held, at least with the current implementation.. Brian > The thing we need to be careful of this is minimising the lock hold > time by the AIL while we unpin the item. That's the "long tail > latencies" problem I mention above. Essentially, we need to try to > avoid holding the item locked for a long period of time before > issuing the log force and/or resubmitting it for IO once it is > unlocked. I have a few ideas on this, but no patches yet. > > Cheers, > > Dave. > -- > Dave Chinner > david@xxxxxxxxxxxxx >
