Re: [PATCH 5/5] xfs: embedd struct xfbtree into the owning structure

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

On Wed, Jan 03, 2024 at 11:22:00PM -0800, Darrick J. Wong wrote:
> On Thu, Jan 04, 2024 at 08:17:35AM +0100, Christoph Hellwig wrote:
> > On Wed, Jan 03, 2024 at 11:14:54PM -0800, Darrick J. Wong wrote:
> > > IIRC setting up the shrinker in xfs_alloc_buftarg_common takes some
> > > shrinker lock somewhere, and lockdep complained about a potential
> > > deadlock between the locks that scrub takes if I don't create the xfile
> > > buftarg in the scrub _setup routines.  That's why it's not created
> > > internally to the xfbtree.
> > > 
> > > I agree that it makes much more sense only to create those things when
> > > they're actually needed, but ... hm.  Maybe we don't need the xfile
> > > buftarg to be hooked up to the shrinkers, seeing as it's ephemeral
> > > anyway?  That would save a lot of fuss and ...
> > 
> > Yes, once we move to a model where the buffer always points to
> > the shmem page, and we remove the buffer lru for them as we already
> > have the page LRU there is no point in having a shrinker at all.
> 
> Ok.  I'll move the shrinker stuff into the real buftarg creation
> function.  This seems to have become a lot simpler now that the shrinker
> is a pointer instead of embedded in the buftarg.

Though looking at buftarg allocation and my old notes from a couple of
years ago -- a second reason for allocating the buftarg during scrub
setup was that the list_lru_init call allocates an array that's
O(nodes_nr) and percpu_counter_init allocates an array that's
O(maxcpus).  At the time I decided that it was better to put those large
contiguous memory allocations in the ->setup routine where we don't have
any vfs/xfs locks held, can run direct reclaim, and haven't done any xfs
work yet.

So I'd like to leave the buftarg attached to struct xfs_scrub, though
I'll still get rid of the shrinker for xfile buftargs.

--D

> > > > naming and moving it out of scrub/ would make sense as the concept
> > > > isn't really scrub/repair specific.  But if we want to stick with
> > > > it I'd prefer to not also have _mem-based naming.
> > > 
> > > Yes, lets move it to libxfs/xfbtree.[ch].
> > 
> > What does the xf in the various scrubx/xf* thinks stand for, btw?
> > Why not libxfs/xfs_btree_mem.[ch]?
> 
> "xfile btree".
> 
> --D
>