On Thu, May 22, 2025 at 10:07:13PM -0700, Christoph Hellwig wrote: > First a note: I gave up on my previous rushed attempt to review this > because the patch was just too big for my limited mental capacity. > > Today I instead split out the relatively trivial prep parts to focus > on the changes. I've pushed my split to: > > git://git.infradead.org/users/hch/xfsprogs.git shutdown-deadlock-split I'll look at it next week - after a week of solid rain here (lots of severe flooding in the local area) I've got a lot of work to do over the next couple of days to do. And I've still got a dozen trees on the ground from the last severe rain event that happened 3 weeks ago to finish making safe... > it would be great to pick that up or do something similar for the next > round. After your patch I've also added two cosmetic patches. I did mention that I needed to split it :) > With that the change does look good to me, and the bli reference > count scheme looks almost sane to me. Although it makes me wonder > if we should just keep a BLI reference for any BLI in the AIL to > further simplify it eventually? I've looked at that in the past - the first time I tried to fix this years ago I ran away to retain my sanity. There are some nasty gotchas... They start with several pieces of code that assumed that a BLI with no references but is dirty must be in the AIL and so is safe to access and modify. Similarly, the buffer unpin code (and now the transaction release code) assume that the AIL doesn't hold a reference so the last reference on stale BLIs will trigger at journal commit complete/trans commit completion time, allowing the BLI to be removed from the AIL, torn down, and the buffer cleaned up left in a state where it can be reused safely from the next lookup. However, if the BLI has an AIL reference, neither of these things will trigger cleanup if the buffer is already in the AIL. At this point, we end up with stale buffers over freed space in the AIL that only AIL pushing can remove. It does no such thing right now. If we then have reallocation of the freed metadata block extent and lookup of the buffer, the stale flag will then be cleared on it. At this point, we have a stale BLI and a non-stale buffer, and stuff just gets worse from there. We even risk writing buffer contents from the AIL whilst it is free in the journal but only allocated in memory.... IOWs, it is not that simple, nor is it risk free to give the BLI in the AIL an active reference.... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
