On Fri, Sep 22, 2023 at 11:04:38AM +1000, Dave Chinner wrote:
> 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@xxxxxxxxxx>
> > >
> > > Because we are going to use the same list-based discard submission
> > > interface for fstrim-based discards, too.
> > >
> > > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ....
> > > @@ -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....
ouch.
> > 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....
<nod>
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--D
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
