On Tue, Mar 03, 2020 at 11:09:09AM +1100, Dave Chinner wrote:
> On Mon, Mar 02, 2020 at 02:00:34PM -0500, Brian Foster wrote:
> > 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@xxxxxxxxxx>
> > > .....
> > > > /*
> > > > @@ -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.
>
> Ah. Comment please :P
>
Sure.
> > > > + 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.
>
> I think I'd prefer to keep things simple right now. Making it an
> unconditional callout keeps this code simple, and if there's a
> common implementation, add a generic function for it that the items
> use.
>
Fine by me, will fix.
Brian
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david@xxxxxxxxxxxxx
>
