On Fri, Jan 14, 2022 at 09:35:35AM -0800, Darrick J. Wong wrote:
> On Fri, Jan 14, 2022 at 09:38:10AM +1100, Dave Chinner wrote:
> > On Thu, Jan 13, 2022 at 08:37:01AM -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.
> > >
> > > 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 internal blockgc reclaim during
> > > the freeze sequence to guarantee that inode eviction doesn't lead to
> > > this state due to eofb or cowblocks inodes. This is similar to
> > > current behavior on read-only remount. 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>
> > > ---
> > > fs/xfs/xfs_super.c | 19 +++++++++++++++++--
> > > 1 file changed, 17 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index c7ac486ca5d3..1d0f87e47fa4 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > > @@ -623,8 +623,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(
> > > @@ -764,6 +769,16 @@ xfs_fs_sync_fs(
> > > * when the state is either SB_FREEZE_FS or SB_FREEZE_COMPLETE.
> > > */
> > > if (sb->s_writers.frozen == SB_FREEZE_PAGEFAULT) {
> > > + struct xfs_icwalk icw = {0};
> > > +
> > > + /*
> > > + * Clear out eofb and cowblocks inodes so eviction while frozen
> > > + * doesn't leave them sitting in the inactivation queue where
> > > + * they cannot be processed.
> > > + */
> > > + icw.icw_flags = XFS_ICWALK_FLAG_SYNC;
> > > + xfs_blockgc_free_space(mp, &icw);
> >
> > Is a SYNC walk safe to run here? I know we run
> > xfs_blockgc_free_space() from XFS_IOC_FREE_EOFBLOCKS under
> > SB_FREEZE_WRITE protection, but here we have both frozen writes and
> > page faults we're running in a much more constrained freeze context
> > here.
> >
> > i.e. the SYNC walk will keep busy looping if it can't get the
> > IOLOCK_EXCL on an inode that is in cache, so if we end up with an
> > inode locked and blocked on SB_FREEZE_WRITE or SB_FREEZE_PAGEFAULT
> > for whatever reason this will never return....
>
> Are you referring to the case where one could be read()ing from a file
> into a buffer that's really a mmap'd page from another file while the
> underlying fs is being frozen?
>
I thought this was generally safe as freeze protection sits outside of
the locks, but I'm not terribly sure about the read to a mapped buffer
case. If that allows an iolock holder to block on a pagefault due to
freeze, then SB_FREEZE_PAGEFAULT might be too late for a synchronous
scan (i.e. assuming nothing blocks this earlier or prefaults/pins the
target buffer)..?
> Also, I added this second patch and fstests runtime went up by 30%.
> ISTR Dave commenting that freeze time would go way up when I submitted a
> patch to clean out the cow blocks a few years ago.
>
Do you have any more detailed data on this? I.e., is this an increase
across the board? A smaller set of tests doing many freezes with a
significant runtime increase?
I'm a little on the fence about this because personally, I'm not
terribly concerned about the performance of a single freeze call. At the
same time, I could see this adding up across hundreds of cycles or
whatever via a test or test suite, and that being potentially annoying
to devs/testers.
> Also also looking through the archives[1], Brian once commented that
> cleaning up all this stuff should be done /if/ one decides to mount the
> frozen-snapshot writable at some later point in time.
>
That kind of sounds like the tradeoff of impacting the current/active fs
for the benefit of a snapshot that may or may not be used. If not, then
it's a waste of time. If so, it might be beneficial for the snap to more
accurately reflect the "eventual" state of the original. For cowblocks
perhaps it doesn't matter if the mount/recovery will scan and reclaim.
I'm not as sure for eofblocks, wouldn't the snap persist all that
"intended to be transient" speculative prealloc until/unless said files
are reopened/closed?
> Maybe this means we ought to find a way to remove inodes from the percpu
> inactivation lists? iget used to be able to pry inodes out of deferred
> inactivation...
>
Seems a reasonable option. Presumably that mitigates the lookup stalling
behavior without the need for any additional scanning work at freeze
time (and maybe eliminates the need for an inodegc flush too), but is
neutral wrt some of the other tradeoffs (like the above). I think the
former is the big question wrt to deferred inactivation whereas the
latter has been the case forever.
BTW, should we care at all about somebody dropping the entire cached
working set (worst case) onto these queues if the fs is frozen? Maybe
not if we have to cycle through all these inodes anyways for a full
working set eviction, and presumably a large evictor task (i.e.
drop_caches) will minimize the percpu queue distribution...
Brian
> [1] https://lore.kernel.org/linux-xfs/20190117181406.GF37591@bfoster/
>
> --D
>
> > Cheers,
> >
> > Dave.
> > --
> > Dave Chinner
> > david@xxxxxxxxxxxxx
>
