Re: [PATCH v2 2/6] xfs: defer agfl block frees when dfops is available

[Brian Foster <bfoster@redhat.com>]

On Tue, May 08, 2018 at 11:04:02AM -0700, Darrick J. Wong wrote:
> On Tue, May 08, 2018 at 08:31:40AM -0400, Brian Foster wrote:
> > On Mon, May 07, 2018 at 06:10:32PM -0700, Darrick J. Wong wrote:
> > > On Wed, Apr 18, 2018 at 09:31:15AM -0400, Brian Foster wrote:
> > ...
> > > > 
> > > > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > > > ---
> > > >  fs/xfs/libxfs/xfs_alloc.c  | 51 ++++++++++++++++++++++++++++++---
> > > >  fs/xfs/libxfs/xfs_defer.h  |  1 +
> > > >  fs/xfs/xfs_trace.h         |  2 ++
> > > >  fs/xfs/xfs_trans.c         | 11 ++++++--
> > > >  fs/xfs/xfs_trans.h         |  1 +
> > > >  fs/xfs/xfs_trans_extfree.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  6 files changed, 129 insertions(+), 7 deletions(-)
> > > > 
> > ...
> > > > diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> > > > index 9d542dfe0052..834388c2c9de 100644
> > > > --- a/fs/xfs/xfs_trans.h
> > > > +++ b/fs/xfs/xfs_trans.h
> > > > @@ -111,6 +111,7 @@ typedef struct xfs_trans {
> > > >  	struct xlog_ticket	*t_ticket;	/* log mgr ticket */
> > > >  	struct xfs_mount	*t_mountp;	/* ptr to fs mount struct */
> > > >  	struct xfs_dquot_acct   *t_dqinfo;	/* acctg info for dquots */
> > > > +	struct xfs_defer_ops	*t_agfl_dfops;	/* optional agfl fixup dfops */
> > > 
> > > This more or less looks fine to me, with ^^^^ this one (incoherent) quibble:
> > > 
> > 
> > I wouldn't call it incoherent...
> > 
> > > A thread can only have one transaction and one dfops in play at a time,
> > > so could we just call this t_dfops and allow anyone to access it who
> > > wants to add their own deferred operations?  Then we can stop passing
> > > both tp and dfops all over the stack.  Can you think of a situation
> > > where we would want access to t_dfops for deferred agfl frees but not
> > > for any other deferred ops?
> > > 
> > 
> > ... because that is precisely the secondary goal of this approach. :)
> > Dave suggested this approach in a previous version and the last post
> > actually used ->t_dfops rather than ->t_agfl_dfops and included some
> > associated dfops parameter elimination cleanup I did along the way.
> > 
> > Christoph objected to the partial refactoring, however, so I'm trying to
> > do this in two parts where the first is primarily focused on fixing the
> > problems resolved by deferred agfl frees. The challenge is that I don't
> > want to manually plumb dfops through new codepaths to control deferred
> > agfl frees only to immediately turn around and clean that code out, and
> > I certainly don't want to refactor all of our dfops code just to enable
> > deferred agfl frees because it creates an unnecessary dependency for
> > backports. As a result, I've stripped out as much refactoring as
> > possible to try and make it clear that this is a bug fix series that
> > happens to create a new xfs_trans field that will open wider cleanups in
> > a follow on series.
> > 
> > IOW, I have a series in progress where patch 1 renames ->t_agfl_dfops
> > back to ->t_dfops to essentially open it for general use and then starts
> > to incorporate the broader cleanups like removing dfops from callstacks,
> > alloc/bmap data structures, etc.
> 
> Seems reasonable.  I've occasionally thought that passing around (tp,
> dfops, firstblock) is kinda bulky and silly.
> 
> > BTW, something I'm curious of your (and Dave, Christoph?) opinion on...
> > another thing that was dropped (permanently) from the previous series
> > was an update to xfs_defer_init() to do the ->t_dfops = &dfops
> > assignment that is otherwise open-coded throughout here. While I don't
> > have a strong preference on that for this series, the more broader
> > cleanup I do the less I like the open-coded approach because it creates
> > a tx setup requirement that's easy to miss (case in point: a function
> > signature change is an easy way to verify all dfops have been associated
> > with transactions). Instead, I'd prefer code that looks something like
> > the following as the end result of the follow-on cleanup series:
> > 
> > 	struct xfs_trans	*tp;
> > 	struct xfs_defer_ops	dfops;
> > 
> > 	xfs_trans_alloc(... &tp);
> > 	xfs_defer_init(&dfops, &first_block, tp);
> > 
> > 	...
> > 
> > 	xfs_defer_finish(&tp);
> > 	xfs_trans_commit(tp);
> > 	...
> > 
> > Note that once we require association of dfops with tp, we don't have to
> > pass dfops around to anywhere the tp isn't already needed, including
> > xfs_defer_finish(). This also opens the possibility of doing things like
> > replacing on-stack dfops with something that's just embedded in
> > xfs_trans itself (optionally pushing down the init/finish calls into the
> > trans infrastructure), but that would require changing how dfops are
> > transferred to rolled transactions and whatnot. I haven't really got far
> > enough to give that proper consideration, but that may be a possible 3rd
> > step cleanup if there's value.
> 
> Not sure.  There are places where it seems to make sense to be able to
> _defer_finish but still have a transaction around to continue making
> changes afterwards.  Or at least, the attr and quota code seems to do
> that. :P
> 

Right.. I do think xfs_defer_finish() should continue to exist even if
we were to eventually do something like embed a dfops in the tp and
allow a transaction commit to do the dfops completion. Something like
that would just essentially clean out some of the simple trans_alloc()
-> do_stuff() -> xfs_defer_finish() -> xfs_trans_commit() boilerplate
code. Code that does looping xfs_defer_finish() calls and whatnot should
continue to look the same.

But I don't think that has much of a bearing on tweaking
xfs_defer_init() vs. tp->t_dfops = &dfops..?

> Also, there's one funny case where we can have a dfops but no
> transaction, which is xlog_recover_process_intents ->
> xlog_finish_defer_ops.  We create a dfops to collect new intents created
> as part of replaying unfinished intents that were in the dirty log, but
> since the intent recovery functions create and commit their own
> transactions, we don't create the transaction that goes with the dfops
> until we're done replaying old intents and ready to finish the new
> intents.

I should have pointed out that the tp parameter is intended to be
optional. So xfs_defer_init() simply does the tp->t_dfops = &dfops
assignment if tp != NULL. If tp == NULL, the associated dfops could
certainly already be associated with some transaction and be reused
(which appears to be the use case down in some of the xfs_da_args code
IIRC), or not at all. So the recovery code you refer to could simply go
unchanged or be an exception where we do open-code the ->t_dfops
assignment.

The part I'm curious about here is really just whether it's worth
tweaking xfs_defer_init() so we don't have to add an additional
tp->t_dfops = &dfops assignment (almost) everywhere we already call
xfs_defer_init(). I suppose I could try that change after the fact so
it's easier to factor in or out...

Brian

> 
> --D
> 
> 
> > Anyways, I'd really like to avoid having to switch back and forth
> > between the xfs_defer_init() tweak and open-coded assignment if
> > possible. Any thoughts/preferences on either approach?
> > Brian
> > 
> > > Separate cleanup, of course, and sorry if this has already been asked.
> > > 
> > > Other than that this looks ok enough to test,
> > > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > --D
> > > 
> > > 
> > > >  	unsigned int		t_flags;	/* misc flags */
> > > >  	int64_t			t_icount_delta;	/* superblock icount change */
> > > >  	int64_t			t_ifree_delta;	/* superblock ifree change */
> > > > diff --git a/fs/xfs/xfs_trans_extfree.c b/fs/xfs/xfs_trans_extfree.c
> > > > index ab438647592a..f5620796ae25 100644
> > > > --- a/fs/xfs/xfs_trans_extfree.c
> > > > +++ b/fs/xfs/xfs_trans_extfree.c
> > > > @@ -231,9 +231,79 @@ static const struct xfs_defer_op_type xfs_extent_free_defer_type = {
> > > >  	.cancel_item	= xfs_extent_free_cancel_item,
> > > >  };
> > > >  
> > > > +/*
> > > > + * AGFL blocks are accounted differently in the reserve pools and are not
> > > > + * inserted into the busy extent list.
> > > > + */
> > > > +STATIC int
> > > > +xfs_agfl_free_finish_item(
> > > > +	struct xfs_trans		*tp,
> > > > +	struct xfs_defer_ops		*dop,
> > > > +	struct list_head		*item,
> > > > +	void				*done_item,
> > > > +	void				**state)
> > > > +{
> > > > +	struct xfs_mount		*mp = tp->t_mountp;
> > > > +	struct xfs_efd_log_item		*efdp = done_item;
> > > > +	struct xfs_extent_free_item	*free;
> > > > +	struct xfs_extent		*extp;
> > > > +	struct xfs_buf			*agbp;
> > > > +	int				error;
> > > > +	xfs_agnumber_t			agno;
> > > > +	xfs_agblock_t			agbno;
> > > > +	uint				next_extent;
> > > > +
> > > > +	free = container_of(item, struct xfs_extent_free_item, xefi_list);
> > > > +	ASSERT(free->xefi_blockcount == 1);
> > > > +	agno = XFS_FSB_TO_AGNO(mp, free->xefi_startblock);
> > > > +	agbno = XFS_FSB_TO_AGBNO(mp, free->xefi_startblock);
> > > > +
> > > > +	trace_xfs_agfl_free_deferred(mp, agno, 0, agbno, free->xefi_blockcount);
> > > > +
> > > > +	error = xfs_alloc_read_agf(mp, tp, agno, 0, &agbp);
> > > > +	if (!error)
> > > > +		error = xfs_free_agfl_block(tp, agno, agbno, agbp,
> > > > +					    &free->xefi_oinfo);
> > > > +
> > > > +	/*
> > > > +	 * Mark the transaction dirty, even on error. This ensures the
> > > > +	 * transaction is aborted, which:
> > > > +	 *
> > > > +	 * 1.) releases the EFI and frees the EFD
> > > > +	 * 2.) shuts down the filesystem
> > > > +	 */
> > > > +	tp->t_flags |= XFS_TRANS_DIRTY;
> > > > +	efdp->efd_item.li_desc->lid_flags |= XFS_LID_DIRTY;
> > > > +
> > > > +	next_extent = efdp->efd_next_extent;
> > > > +	ASSERT(next_extent < efdp->efd_format.efd_nextents);
> > > > +	extp = &(efdp->efd_format.efd_extents[next_extent]);
> > > > +	extp->ext_start = free->xefi_startblock;
> > > > +	extp->ext_len = free->xefi_blockcount;
> > > > +	efdp->efd_next_extent++;
> > > > +
> > > > +	kmem_free(free);
> > > > +	return error;
> > > > +}
> > > > +
> > > > +
> > > > +/* sub-type with special handling for AGFL deferred frees */
> > > > +static const struct xfs_defer_op_type xfs_agfl_free_defer_type = {
> > > > +	.type		= XFS_DEFER_OPS_TYPE_AGFL_FREE,
> > > > +	.max_items	= XFS_EFI_MAX_FAST_EXTENTS,
> > > > +	.diff_items	= xfs_extent_free_diff_items,
> > > > +	.create_intent	= xfs_extent_free_create_intent,
> > > > +	.abort_intent	= xfs_extent_free_abort_intent,
> > > > +	.log_item	= xfs_extent_free_log_item,
> > > > +	.create_done	= xfs_extent_free_create_done,
> > > > +	.finish_item	= xfs_agfl_free_finish_item,
> > > > +	.cancel_item	= xfs_extent_free_cancel_item,
> > > > +};
> > > > +
> > > >  /* Register the deferred op type. */
> > > >  void
> > > >  xfs_extent_free_init_defer_op(void)
> > > >  {
> > > >  	xfs_defer_init_op_type(&xfs_extent_free_defer_type);
> > > > +	xfs_defer_init_op_type(&xfs_agfl_free_defer_type);
> > > >  }
> > > > -- 
> > > > 2.13.6
> > > > 
> > > > --
> > > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > > the body of a message to majordomo@vger.kernel.org
> > > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html