Re: [ANNOUNCE] xfsprogs for-next updated

[Carlos Maiolino <cem@kernel.org>]

> 
> 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.

Yeah, I do agree. Maybe it's better to go with your initial idea for 6.0, and we
postpone the SB buffer pinning to a later release avoiding people to hit this
issue ASAP?!


> 
> > 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).

Thanks, I'm gonna try it :)

> 
> --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

-- 
Carlos Maiolino