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

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

On Thu, Jan 04, 2024 at 07:32:18AM +0100, Christoph Hellwig wrote:
> On Wed, Jan 03, 2024 at 05:21:33PM -0800, Darrick J. Wong wrote:
> > > -int xfbtree_create(struct xfs_mount *mp, const struct xfbtree_config *cfg,
> > > -		struct xfbtree **xfbtreep);
> > > +int xfbtree_init(struct xfs_mount *mp, struct xfbtree *xfbt,
> > > +		const struct xfs_btree_ops *btree_ops);
> > 
> > Why not pass the xfs_buftarg and the owner into the init function?  It
> > feels a little funny that the callsites are:
> > 
> > 	xfbt->target = buftarg;
> > 	xfbt->owner = agno;
> > 	return xfbtree_init(mp, &xfbt, btree_ops);
> > 
> > vs:
> > 
> > 	return xfbtree_init(mp, &xfbt, buftarg, agno, btree_ops);
> 
> Yes, but..
> 
> The owner assignment should really just move into the caller of the
> helpers, which would clean things up.
> 
> And the target one I need to fully understand, but maybe let's bring
> this up here and ask the question I was going to ask elsewhere after
> doing a bit more research.
> 
> The way the in-memory buftargs work right now look weird to me.
> 
> Why do we keep the target as a separate concept from the xfbtree?
> My logical assumption would be that the xfbtree creates the target
> internally and the caller shouldn't have to bother with it.

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

> This also goes further and makes me wonder why the
> xfs_buf_cache is embdded in the xfile and not just allocated when
> allocating a file-backed buftarg?

...maybe we could actually do it this way.  I'll look into it tomorrow.

> Btw, once you start touching the xfbtree can we think a bit about
> the naming?  Right now we have xfbtree but also a xfs_btree_mem.h,
> which seems very confusing.  I think just doing a xfs_btree_mem
> 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].

--D