Re: [PATCH 1/3] xfs: move log discard work to xfs_discard.c

[Dave Chinner <david@fromorbit.com>]

On Thu, Sep 21, 2023 at 08:52:43AM -0700, Darrick J. Wong wrote:
> On Thu, Sep 21, 2023 at 11:39:43AM +1000, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Because we are going to use the same list-based discard submission
> > interface for fstrim-based discards, too.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
....
> > @@ -31,6 +28,23 @@ struct xfs_extent_busy {
> >  #define XFS_EXTENT_BUSY_SKIP_DISCARD	0x02	/* do not discard */
> >  };
> >  
> > +/*
> > + * List used to track groups of related busy extents all the way through
> > + * to discard completion.
> > + */
> > +struct xfs_busy_extents {
> > +	struct xfs_mount	*mount;
> > +	struct list_head	extent_list;
> > +	struct work_struct	endio_work;
> > +
> > +	/*
> > +	 * Owner is the object containing the struct xfs_busy_extents to free
> > +	 * once the busy extents have been processed. If only the
> > +	 * xfs_busy_extents object needs freeing, then point this at itself.
> > +	 */
> > +	void			*owner;
> > +};
> > +
> >  void
> >  xfs_extent_busy_insert(struct xfs_trans *tp, struct xfs_perag *pag,
> >  	xfs_agblock_t bno, xfs_extlen_t len, unsigned int flags);
> > diff --git a/fs/xfs/xfs_log_cil.c b/fs/xfs/xfs_log_cil.c
> > index 3aec5589d717..c340987880c8 100644
> > --- a/fs/xfs/xfs_log_cil.c
> > +++ b/fs/xfs/xfs_log_cil.c
> > @@ -16,8 +16,7 @@
> >  #include "xfs_log.h"
> >  #include "xfs_log_priv.h"
> >  #include "xfs_trace.h"
> > -
> > -struct workqueue_struct *xfs_discard_wq;
> > +#include "xfs_discard.h"
> >  
> >  /*
> >   * Allocate a new ticket. Failing to get a new ticket makes it really hard to
> > @@ -103,7 +102,7 @@ xlog_cil_ctx_alloc(void)
> >  
> >  	ctx = kmem_zalloc(sizeof(*ctx), KM_NOFS);
> >  	INIT_LIST_HEAD(&ctx->committing);
> > -	INIT_LIST_HEAD(&ctx->busy_extents);
> > +	INIT_LIST_HEAD(&ctx->busy_extents.extent_list);
> 
> I wonder if xfs_busy_extents should have an initializer function to
> INIT_LIST_HEAD and set mount/owner?  This patch and the next one both
> have similar initialization sequences.
> 
> (Not sure if you want to INIT_WORK at the same time?)
> 
> >  	INIT_LIST_HEAD(&ctx->log_items);
> >  	INIT_LIST_HEAD(&ctx->lv_chain);
> >  	INIT_WORK(&ctx->push_work, xlog_cil_push_work);
> > @@ -132,7 +131,7 @@ xlog_cil_push_pcp_aggregate(
> >  
> >  		if (!list_empty(&cilpcp->busy_extents)) {
> >  			list_splice_init(&cilpcp->busy_extents,
> > -					&ctx->busy_extents);
> > +					&ctx->busy_extents.extent_list);
> 
> Hmm.  Should xfs_trans.t_busy and xlog_cil_pcp.busy_extents also get
> converted into xfs_busy_extents objects and a helper written to splice
> two busy_extents lists together?
> 
> (This might be architecture astronauting, feel free to ignore this...)

These two cases are a little bit different - they are just lists of
busy extents and do not need any of the stuff for discards. It
doesn't make a whole lot of sense to make them xfs_busy_extents and
then either have to open code all the places they use to add
".extent_list" or add one line wrappers for list add, splice, and
empty check operations.

It's likely more code than just open coding the extent list access
in the couple of places we need to access it directly...

....

> > @@ -980,8 +909,8 @@ xlog_cil_committed(
> >  
> >  	xlog_cil_ail_insert(ctx, abort);
> >  
> > -	xfs_extent_busy_sort(&ctx->busy_extents);
> > -	xfs_extent_busy_clear(mp, &ctx->busy_extents,
> > +	xfs_extent_busy_sort(&ctx->busy_extents.extent_list);
> > +	xfs_extent_busy_clear(mp, &ctx->busy_extents.extent_list,
> >  			      xfs_has_discard(mp) && !abort);
> 
> Should these two xfs_extent_busy objects take the xfs_busy_extent object
> as an arg instead of the mount and list_head?  It seems strange (both
> here and the next patch) to build up this struct and then pass around
> its individual parts.

xfs_extent_busy_sort(), no. It's just sorting a list of busy
extents, and has nothign to do with discard contexts and it gets
called from transaction freeing context when we abort transactions...

xfs_extent_busy_clear() also gets called from transaction context and
does not do discards - it just passes a list of busy extents to be
cleared.

So we'd have to wrap tp->t_busy up as a xfs_busy_extents
object just so we can pass a xfs_busy_extents object to these
functions, even though we are just using these as list_heads and not
for any other purpose.

Ignoring all the helpers we'd need, I'm also not convinced that the
runtime cost of increasing the struct xfs_trans by 48 bytes with
stuff it will never use is lower than the benefit of reducing the
parameters we pass to one function from 3 to 2....

> The straight conversion aspect of this patch looks correct, so (aside
> from the question above) any larger API cleanups can be their own patch.

If it was a much more widely used API, it might make sense to make
the struct xfs_busy_extents a first class citizen. But as it stands
it's just a wrapper to enable discard operation to be abstracted so
I've just made it as minimally invasive as I can....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com