On Fri, Jan 19, 2024 at 9:35 PM Brian Foster <bfoster@xxxxxxxxxx> 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. > > 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. This was always possible to some degree, but is > potentially amplified by the fact that reclaim no longer blocks on > the first inode that requires inactivation work. Instead, we > populate the inactivation queues indefinitely. The side effect can > be observed easily by invoking drop_caches on a frozen fs previously > populated with eofb and/or cowblocks inodes and then running > anything that relies on inode lookup (i.e., ls). > > To mitigate this behavior, invoke a non-sync blockgc scan during the > freeze sequence to minimize the chance that inode evictions require > inactivation while the fs is frozen. A synchronous scan would > provide more of a guarantee, but is potentially unsafe from > partially frozen context. This is because a file read task may be > blocked on a write fault while holding iolock (such as when reading > into a mapped buffer) and a sync scan retries indefinitely on iolock > failure. Therefore, this adds risk of potential livelock during the > freeze sequence. > > Finally, since the deadlock issue was present for such a long time, > also document the subtle ->destroy_inode() constraint to avoid > unintentional reintroduction of the deadlock problem in the future. > > Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx> Is there an appropriate Fixes: commit that could be mentioned here? or at least a range of stable kernels to apply this suggested fix? Thanks, Amir. > --- > > Hi all, > > There was a good amount of discussion on the first version of this patch > [1] a couple or so years ago now. The main issue was that using a sync > scan is unsafe in certain cases (best described here [2]), so this > best-effort approach was considered as a fallback option to improve > behavior. > > The reason I'm reposting this is that it is one of several options for > dealing with the aforementioned deadlock on stable/distro kernels, so it > seems to have mutual benefit. Looking back through the original > discussion, I think there are several ways this could be improved to > provide the benefit of a sync scan. For example, if the scan could be > made to run before faults are locked out (re [3]), that may be > sufficient to allow a sync scan. Or now that freeze_super() actually > checks for ->sync_fs() errors, an async scan could be followed by a > check for tagged blockgc entries that triggers an -EBUSY or some error > return to fail the freeze, which would most likely be a rare and > transient situation. Etc. > > These thoughts are mainly incremental improvements upon some form of > freeze time scan and may not be of significant additional value given > current upstream behavior, so this patch takes the simple, best effort > approach. Thoughts? > > Brian > > [1] https://lore.kernel.org/linux-xfs/20220113133701.629593-1-bfoster@xxxxxxxxxx/ > [2] https://lore.kernel.org/linux-xfs/20220115224030.GA59729@xxxxxxxxxxxxxxxxxxx/ > [3] https://lore.kernel.org/linux-xfs/Yehvc4g+WakcG1mP@bfoster/ > > fs/xfs/xfs_super.c | 24 ++++++++++++++++-------- > 1 file changed, 16 insertions(+), 8 deletions(-) > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index d0009430a627..43e72e266666 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -657,8 +657,13 @@ xfs_fs_alloc_inode( > } > > /* > - * Now that the generic code is guaranteed not to be accessing > - * the linux inode, we can inactivate and reclaim the inode. > + * Now that the generic code is guaranteed not to be accessing the inode, we can > + * inactivate and reclaim it. > + * > + * NOTE: ->destroy_inode() can be called (with ->s_umount held) while the > + * filesystem is frozen. Therefore it is generally unsafe to attempt transaction > + * allocation in this context. A transaction alloc that blocks on frozen state > + * from a context with ->s_umount held will deadlock with unfreeze. > */ > STATIC void > xfs_fs_destroy_inode( > @@ -811,15 +816,18 @@ xfs_fs_sync_fs( > * down inodegc because once SB_FREEZE_FS is set it's too late to > * prevent inactivation races with freeze. The fs doesn't get called > * again by the freezing process until after SB_FREEZE_FS has been set, > - * so it's now or never. Same logic applies to speculative allocation > - * garbage collection. > + * so it's now or never. > * > - * We don't care if this is a normal syncfs call that does this or > - * freeze that does this - we can run this multiple times without issue > - * and we won't race with a restart because a restart can only occur > - * when the state is either SB_FREEZE_FS or SB_FREEZE_COMPLETE. > + * The same logic applies to block garbage collection. Run a best-effort > + * blockgc scan to reduce the working set of inodes that the shrinker > + * would send to inactivation queue purgatory while frozen. We can't run > + * a sync scan with page faults blocked because that could potentially > + * livelock against a read task blocked on a page fault (i.e. if reading > + * into a mapped buffer) while holding iolock. > */ > if (sb->s_writers.frozen == SB_FREEZE_PAGEFAULT) { > + xfs_blockgc_free_space(mp, NULL); > + > xfs_inodegc_stop(mp); > xfs_blockgc_stop(mp); > } > -- > 2.42.0 > >
