Re: [ANNOUNCE] xfsprogs for-next updated

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Aug 31, 2022 at 11:43:25AM +0200, Carlos Maiolino wrote:
> On Tue, Aug 30, 2022 at 08:12:17AM -0700, Darrick J. Wong wrote:
> > On Tue, Aug 30, 2022 at 01:52:20PM +0200, Carlos Maiolino wrote:
> > > Hi folks,
> > >
> > > The for-next branch of the xfsprogs repository at:
> > >
> > >         git://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git
> > >
> > > has just been updated.
> > >
> > > This update contains the initial libxfs sync to Linux 6.0 and should be turned
> > > into -rc0 once it (hopefully) gets some testing (and no complains) for more people.
> > 
> > Wooo, welcome, new maintainer! :)
> 
> \o/
> 
> > 
> > > Please, if any questions, let me know.
> > 
> > For the repair deadlock fix[1], do you want me to pin the primary
> > superblock buffer to the xfs_mount like Dave suggested in [2]?
> 
> I'd rather have it pinned to the xfs_mount as it's often accessed, do you think
> it is doable (you mentioned you've ran into many problems with that)?

Oh, the usual problems of adding a new interface...

1. Who is responsible for setting m_sb_bp?

Should libxfs_mount attach m_sb_bp?  Should individual programs decide
to do that if they require the functionality?  Should we instead have a
xfs_getsb function that returns m_sb_bp if set, or libxfs_getbufr's a
new buffer and tries to cmpxchg it with the pointer?

What about mkfs, which needs to libxfs_mount before it's even written
anything to disk?

2. Should it be a cached buffer so that any other program (e.g. xfs_db)
doing open-coded accesses of the superblock will get the same cached
buffer, or should it be uncached like the kernel?

If we decide on uncached, this will necessitate a full audit of xfsprogs
to catch open-coded calls to libxfs_getbuf for the primary super, or
else coherency problems will result.

If we decide on using a cached buffer and setting it in libxfs_mount,
then the part of xfs_repair that tears down the buffer cache and
reinitializes it with a different hash size will also have to learn to
brelse m_sb_bp before destroying the cache and re-assign it afterwards.
Alternately, I suppose it could learn to rehash itself.

This is a /lot/ to think about to solve one problem in one program.

> I didn't have time to try to reproduce those deadlocks yet though.

If you modify cache_node_get like this to make reclaim more aggressive:

diff --git a/libxfs/cache.c b/libxfs/cache.c
index 139c7c1b..b5e1bcf8 100644
--- a/libxfs/cache.c
+++ b/libxfs/cache.c
@@ -448,10 +448,10 @@ cache_node_get(
 		/*
 		 * not found, allocate a new entry
 		 */
+		priority = cache_shake(cache, priority, false);
 		node = cache_node_allocate(cache, key);
 		if (node)
 			break;
-		priority = cache_shake(cache, priority, false);
 		/*
 		 * We start at 0; if we free CACHE_SHAKE_COUNT we get
 		 * back the same priority, if not we get back priority+1.

It's trivially reproducible with xfs_repair (do not specify -n).

--D

> > 
> > [1] https://lore.kernel.org/linux-xfs/166007921743.3294543.7334567013352169774.stgit@magnolia/
> > [2] https://lore.kernel.org/linux-xfs/20220811221541.GQ3600936@dread.disaster.area/
> 
> -- 
> Carlos Maiolino