Re: [PATCH 01/12] xfs: refactor failed buffer resubmission into xfsaild

[Brian Foster <bfoster@redhat.com>]

On Mon, Apr 20, 2020 at 12:45:56PM +1000, Dave Chinner wrote:
> On Fri, Apr 17, 2020 at 11:08:48AM -0400, Brian Foster wrote:
> > Flush locked log items whose underlying buffers fail metadata
> > writeback are tagged with a special flag to indicate that the flush
> > lock is already held. This is currently implemented in the type
> > specific ->iop_push() callback, but the processing required for such
> > items is not type specific because we're only doing basic state
> > management on the underlying buffer.
> > 
> > Factor the failed log item handling out of the inode and dquot
> > ->iop_push() callbacks and open code the buffer resubmit helper into
> > a single helper called from xfsaild_push_item(). This provides a
> > generic mechanism for handling failed metadata buffer writeback with
> > a bit less code.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> .....
> > diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> > index 564253550b75..0c709651a2c6 100644
> > --- a/fs/xfs/xfs_trans_ail.c
> > +++ b/fs/xfs/xfs_trans_ail.c
> > @@ -345,6 +345,45 @@ xfs_ail_delete(
> >  	xfs_trans_ail_cursor_clear(ailp, lip);
> >  }
> >  
> > +/*
> > + * Requeue a failed buffer for writeback.
> > + *
> > + * We clear the log item failed state here as well, but we have to be careful
> > + * about reference counts because the only active reference counts on the buffer
> > + * may be the failed log items. Hence if we clear the log item failed state
> > + * before queuing the buffer for IO we can release all active references to
> > + * the buffer and free it, leading to use after free problems in
> > + * xfs_buf_delwri_queue. It makes no difference to the buffer or log items which
> > + * order we process them in - the buffer is locked, and we own the buffer list
> > + * so nothing on them is going to change while we are performing this action.
> > + *
> > + * Hence we can safely queue the buffer for IO before we clear the failed log
> > + * item state, therefore  always having an active reference to the buffer and
> > + * avoiding the transient zero-reference state that leads to use-after-free.
> > + */
> > +static inline int
> > +xfsaild_push_failed(
> 
> Bad name IMO. Makes me think it's an action that is taken when an
> item push failed, not an action that resubmits a buffer that had an
> IO failure.
> 
> i.e. "push" doesn't imply IO, and failure to push an item doesn't
> mean there was an IO error - it may be locked, already flushing,
> pinned, etc.
> 

Yeah..

> > +	struct xfs_log_item	*lip,
> > +	struct list_head	*buffer_list)
> > +{
> > +	struct xfs_buf		*bp = lip->li_buf;
> 
> This also assumes that the log item has a buffer associated with it.
> So perhaps "xfsaild_resubmit_failed_buffer()" would be more
> approriate here.
> 

The buffer pointer is an implementation detail of failed items. It would
be nice to find a way to avoid that. How about xfsaild_resubmit_item()
to be consistent with the parent function (xfsaild_push_item())?

Brian

> Other than that, the code is fine.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
>