Re: [RFC v5 PATCH 4/9] xfs: automatic relogging item management

[Dave Chinner <david@fromorbit.com>]

On Thu, Feb 27, 2020 at 08:43:16AM -0500, Brian Foster wrote:
> As implemented by the previous patch, relogging can be enabled on
> any item via a relog enabled transaction (which holds a reference to
> an active relog ticket). Add a couple log item flags to track relog
> state of an arbitrary log item. The item holds a reference to the
> global relog ticket when relogging is enabled and releases the
> reference when relogging is disabled.
> 
> Signed-off-by: Brian Foster <bfoster@redhat.com>
> ---
>  fs/xfs/xfs_trace.h      |  2 ++
>  fs/xfs/xfs_trans.c      | 36 ++++++++++++++++++++++++++++++++++++
>  fs/xfs/xfs_trans.h      |  6 +++++-
>  fs/xfs/xfs_trans_priv.h |  2 ++
>  4 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index a86be7f807ee..a066617ec54d 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -1063,6 +1063,8 @@ DEFINE_LOG_ITEM_EVENT(xfs_ail_push);
>  DEFINE_LOG_ITEM_EVENT(xfs_ail_pinned);
>  DEFINE_LOG_ITEM_EVENT(xfs_ail_locked);
>  DEFINE_LOG_ITEM_EVENT(xfs_ail_flushing);
> +DEFINE_LOG_ITEM_EVENT(xfs_relog_item);
> +DEFINE_LOG_ITEM_EVENT(xfs_relog_item_cancel);
>  
>  DECLARE_EVENT_CLASS(xfs_ail_class,
>  	TP_PROTO(struct xfs_log_item *lip, xfs_lsn_t old_lsn, xfs_lsn_t new_lsn),
> diff --git a/fs/xfs/xfs_trans.c b/fs/xfs/xfs_trans.c
> index 8ac05ed8deda..f7f2411ead4e 100644
> --- a/fs/xfs/xfs_trans.c
> +++ b/fs/xfs/xfs_trans.c
> @@ -778,6 +778,41 @@ xfs_trans_del_item(
>  	list_del_init(&lip->li_trans);
>  }
>  
> +void
> +xfs_trans_relog_item(
> +	struct xfs_log_item	*lip)
> +{
> +	if (!test_and_set_bit(XFS_LI_RELOG, &lip->li_flags)) {
> +		xfs_trans_ail_relog_get(lip->li_mountp);
> +		trace_xfs_relog_item(lip);
> +	}

What if xfs_trans_ail_relog_get() fails to get a reference here
because there is no current ail relog ticket? Isn't the transaction
it was reserved in required to be checked here for XFS_TRANS_RELOG
being set?

> +}
> +
> +void
> +xfs_trans_relog_item_cancel(
> +	struct xfs_log_item	*lip,
> +	bool			drain) /* wait for relogging to cease */
> +{
> +	struct xfs_mount	*mp = lip->li_mountp;
> +
> +	if (!test_and_clear_bit(XFS_LI_RELOG, &lip->li_flags))
> +		return;
> +	xfs_trans_ail_relog_put(lip->li_mountp);
> +	trace_xfs_relog_item_cancel(lip);
> +
> +	if (!drain)
> +		return;
> +
> +	/*
> +	 * Some operations might require relog activity to cease before they can
> +	 * proceed. For example, an operation must wait before including a
> +	 * non-lockable log item (i.e. intent) in another transaction.
> +	 */
> +	while (wait_on_bit_timeout(&lip->li_flags, XFS_LI_RELOGGED,
> +				   TASK_UNINTERRUPTIBLE, HZ))
> +		xfs_log_force(mp, XFS_LOG_SYNC);
> +}

What is a "cancel" operation? Is it something you do when cancelling
a transaction (i.e. on operation failure) or is is something the
final transaction does to remove the relog item from the AIL (i.e.
part of the normal successful finish to a long running transaction)?


>  /* Detach and unlock all of the items in a transaction */
>  static void
>  xfs_trans_free_items(
> @@ -863,6 +898,7 @@ xfs_trans_committed_bulk(
>  
>  		if (aborted)
>  			set_bit(XFS_LI_ABORTED, &lip->li_flags);
> +		clear_and_wake_up_bit(XFS_LI_RELOGGED, &lip->li_flags);

I don't know what the XFS_LI_RELOGGED flag really means in this
patch because I don't know what sets it. Perhaps this would be
better moved into the patch that first sets the RELOGGED flag?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com