On Mon, Apr 20, 2020 at 09:58:25AM -0400, Brian Foster wrote:
> 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@xxxxxxxxxx>
> > .....
> > > 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())?
That works, too. :)
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
