On Mon, Jan 29, 2024 at 10:02:16AM -0500, Brian Foster wrote: > On Fri, Jan 26, 2024 at 10:46:12AM +1100, Dave Chinner wrote: > > On Thu, Jan 25, 2024 at 07:46:55AM -0500, Brian Foster wrote: > > > On Mon, Jan 22, 2024 at 02:23:44PM +1100, Dave Chinner wrote: > > > > On Fri, Jan 19, 2024 at 02:36:45PM -0500, Brian Foster wrote: > > > > > We've had reports on distro (pre-deferred inactivation) kernels that > > > > > inode reclaim (i.e. via drop_caches) can deadlock on the s_umount > > > > > lock when invoked on a frozen XFS fs. This occurs because > > > > > drop_caches acquires the lock and then blocks in xfs_inactive() on > > > > > transaction alloc for an inode that requires an eofb trim. unfreeze > > > > > then blocks on the same lock and the fs is deadlocked. > > > > > > > > Yup, but why do we need to address that in upstream kernels? > > > > > > > > > With deferred inactivation, the deadlock problem is no longer > > > > > present because ->destroy_inode() no longer blocks whether the fs is > > > > > frozen or not. There is still unfortunate behavior in that lookups > > > > > of a pending inactive inode spin loop waiting for the pending > > > > > inactive state to clear, which won't happen until the fs is > > > > > unfrozen. > > > > > > > > Largely we took option 1 from the previous discussion: > > > > > > > > | ISTM the currently most viable options we've discussed are: > > > > | > > > > | 1. Leave things as is, accept potential for lookup stalls while frozen > > > > | and wait and see if this ever really becomes a problem for real users. > > > > > > > > https://lore.kernel.org/linux-xfs/YeVxCXE6hXa1S%2Fic@bfoster/ > > > > > > > > And really it hasn't caused any serious problems with the upstream > > > > and distro kernels that have background inodegc. > > > > > > > > > > For quite a long time, neither did introduction of the reclaim s_umount > > > deadlock. > > > > Yup, and that's *exactly* the problem we should be fixing here > > because that's the root cause of the deadlock you are trying to > > mitigate with these freeze-side blockgc flushes. > > > > The deadlock in XFS inactivation is only the messenger - it's > > a symptom of the problem, and trying to prevent inactivation in that > > scenario is only addressing one specific symptom. It doesn't > > address any other potential avenue to the same deadlock in XFS or > > in any other filesystem that can be frozen. > > We address symptoms all the time. You've suggested several alternatives > in this thread that also only address symptoms. I see no reason why > symptoms and the cure must be addressed at the same time. > > > > I can only speak for $employer distros of course, but my understanding > > > is that the kernels that do have deferred inactivation are still in the > > > early adoption phase of the typical lifecycle. > > > > Fixing the (shrinker) s_umount vs thaw deadlock is relevant to > > current upstream kernels because it removes a landmine that any > > filesystem could step on. It is also a fix that could be backported > > to all downstream kernels, and in doing so will also solve the > > issue on the distro you care about.... > > > > I don't think a novel upstream cross subsystem lock split exercise is > the most practical option for a stable kernel. No, but that's not the point. If you need a custom fix for backporting to older kernels, then the first patch in the series is the custom fix, then it gets followed by the changes that fix the remaining upstream issues properly. Then the last patch in the series removes the custom hack for backports (if it still exists). We know how to do this - we've done it many times in the past - and it's a win-win because everyone gets what they want. i.e. There's a backportable fix for stable kernels that doesn't burden upstream, and the upstream issues are addressed in the best possible way and we don't leave technical debt behind that upstream developers will still have to address at some point in the future. > > I started on the VFS changes just before christmas, but I haven't > > got back to it yet because it wasn't particularly high priority. The > > patch below introduces the freeze serialisation lock, but doesn't > > yet reduce the s_umount scope of thaw. Maybe you can pick it up from > > here? > > > > I don't disagree with this idea or as a general direction, but there's > too much additional risk, complexity and unknown here that I don't think > this is the best next step. My preference is to improve on problematic > behaviors with something like the async scan patch, unlink the user > reported issue(s) from the broader design flaw(s), and then address the > latter as a follow on effort. I tire of discussions about how we *might* do something. "Deeds not words", to quote my high school's motto. Here's the fixes for the iget vs inactive vs freeze problems in the upstream kernel: https://lore.kernel.org/linux-xfs/20240201005217.1011010-1-david@xxxxxxxxxxxxx/T/#t With that sorted, are there any other issues we know about that running a blockgc scan during freeze might work around? -Dave. -- Dave Chinner david@xxxxxxxxxxxxx
