Re: [RFC v5 PATCH 7/9] xfs: buffer relogging support prototype

[Brian Foster <bfoster@redhat.com>]

On Mon, Mar 02, 2020 at 06:47:28PM +1100, Dave Chinner wrote:
> On Thu, Feb 27, 2020 at 08:43:19AM -0500, Brian Foster wrote:
> > Add a quick and dirty implementation of buffer relogging support.
> > There is currently no use case for buffer relogging. This is for
> > experimental use only and serves as an example to demonstrate the
> > ability to relog arbitrary items in the future, if necessary.
> > 
> > Add a hook to enable relogging a buffer in a transaction, update the
> > buffer log item handlers to support relogged BLIs and update the
> > relog handler to join the relogged buffer to the relog transaction.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> .....
> >  /*
> > @@ -187,9 +188,21 @@ xfs_ail_relog(
> >  			xfs_log_ticket_put(ailp->ail_relog_tic);
> >  		spin_unlock(&ailp->ail_lock);
> >  
> > -		xfs_trans_add_item(tp, lip);
> > -		set_bit(XFS_LI_DIRTY, &lip->li_flags);
> > -		tp->t_flags |= XFS_TRANS_DIRTY;
> > +		/*
> > +		 * TODO: Ideally, relog transaction management would be pushed
> > +		 * down into the ->iop_push() callbacks rather than playing
> > +		 * games with ->li_trans and looking at log item types here.
> > +		 */
> > +		if (lip->li_type == XFS_LI_BUF) {
> > +			struct xfs_buf_log_item	*bli = (struct xfs_buf_log_item *) lip;
> > +			xfs_buf_hold(bli->bli_buf);
> 
> What is this for? The bli already has a reference to the buffer.
> 

The buffer reference is for the transaction. It is analogous to the
reference acquired in xfs_buf_find() via xfs_trans_[get|read]_buf(), for
example.

> > +			xfs_trans_bjoin(tp, bli->bli_buf);
> > +			xfs_trans_dirty_buf(tp, bli->bli_buf);
> > +		} else {
> > +			xfs_trans_add_item(tp, lip);
> > +			set_bit(XFS_LI_DIRTY, &lip->li_flags);
> > +			tp->t_flags |= XFS_TRANS_DIRTY;
> > +		}
> 
> Really, this should be a xfs_item_ops callout. i.e.
> 
> 		lip->li_ops->iop_relog(lip);
> 

Yeah, I've already done pretty much this in my local tree. The callback
also takes the transaction because that's the code that knows how to add
a particular type of item to a transaction. I didn't require a callback
for the else case above where no special handling is required
(quotaoff), so the callback is optional, but I'm not opposed to
reworking things such that ->iop_relog() is always required if that is
preferred.

> And then a) it doesn't matter really where we call it from, and b)
> it becomes fully generic and we can implement the callout
> as future functionality requires.
> 

Yep.

Brian

> However, we have to make sure that the current transaction we are
> running has the correct space usage accounted to it, so I think this
> callout really does need to be done in a tight loop iterating and
> accounting all the relog items into the transaction without outside
> interference.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>