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.
> + 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.
Other than that, the code is fine.
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
