On Sat, Jan 20, 2024 at 10:50:02AM +0200, Amir Goldstein wrote: > 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? > Hmm.. well I didn't really consider this a bug upstream. The above is more historical reference to an issue that has since gone away, but trying to use the bug report on a stable kernel to be forward looking about improving on potentially awkward behavior of the latest upstream kernel under the same sort of circumstances (i.e. reclaim while frozen). I suppose something like this would be potentially useful for stable kernels that don't include background inactivation. I haven't audited which stable kernels might fall in that category (if any), but alas it probably doesn't matter because this patch likely wasn't going anywhere anyways. Brian > 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 > > > > >
