On Tue, Aug 27, 2024 at 05:30:49AM -0700, Christoph Hellwig wrote: > On Tue, Aug 27, 2024 at 08:00:05PM +1000, Dave Chinner wrote: > > Hence the only cases where the item might have been already removed > > from the AIL by the ->iop_push() are those where the push itself > > removes the item from the AIL. This only occurs in shutdown > > situations, so it's not the common case. > > > > In which case, returning XFS_ITEM_FREED to tell the push code that > > it was freed and should not reference it at all is fine. We don't > > really even need tracing for this case because if the items can't be > > removed from the AIL, they will leave some other AIL trace when > > pushe (i.e. they will be stuck locked, pinned or flushing and those > > will leave traces...) > > So XFS_ITEM_FREED is definitively a better name, but it still feels > a bit fragile that any of these shutdown paths need special handling > inside ->iop_push. Agreed, but I don't see an easy way to fix that right now because the shutdown behaviour is both item type and item state specific. I suspect that we'd do better to have explicit shutdown processing of log items in the AIL (i.e. a ->iop_shutdown method) that is called instead of ->iop_push when the AIL detects that the filesystem has shut down. We can then define the exact behaviour we want in this case and processing does not have to be non-blocking for performance and latency reasons. If we go down that route, I think we'd want to add a XFS_ITEM_SHUTDOWN return value after the push code calls xfs_force_shutdown(). The push code does not error out the item or remove it from the AIL, just shuts down the fs and returns XFS_ITEM_SHUTDOWN. The AIL then breaks out of the push loop and submits the delwri buffers which will error them all out and remove them from the AIL because the fs is shut down. It then starts a new walk from the tail calling ->iop_shutdown on everything remaining in the AIL until the AIL is empty.... -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
