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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Apr 29, 2020 at 01:21:37PM -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>
> Reviewed-by: Allison Collins <allison.henderson@oracle.com>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Nice little refactoring,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> ---
>  fs/xfs/xfs_buf_item.c   | 39 ---------------------------------------
>  fs/xfs/xfs_buf_item.h   |  2 --
>  fs/xfs/xfs_dquot_item.c | 15 ---------------
>  fs/xfs/xfs_inode_item.c | 15 ---------------
>  fs/xfs/xfs_trans_ail.c  | 41 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 41 insertions(+), 71 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 1545657c3ca0..8796adde2d12 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1248,42 +1248,3 @@ xfs_buf_iodone(
>  	xfs_trans_ail_delete(ailp, lip, SHUTDOWN_CORRUPT_INCORE);
>  	xfs_buf_item_free(BUF_ITEM(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.
> - *
> - * Return true if the buffer was added to the buffer list, false if it was
> - * already on the buffer list.
> - */
> -bool
> -xfs_buf_resubmit_failed_buffers(
> -	struct xfs_buf		*bp,
> -	struct list_head	*buffer_list)
> -{
> -	struct xfs_log_item	*lip;
> -	bool			ret;
> -
> -	ret = xfs_buf_delwri_queue(bp, buffer_list);
> -
> -	/*
> -	 * XFS_LI_FAILED set/clear is protected by ail_lock, caller of this
> -	 * function already have it acquired
> -	 */
> -	list_for_each_entry(lip, &bp->b_li_list, li_bio_list)
> -		xfs_clear_li_failed(lip);
> -
> -	return ret;
> -}
> diff --git a/fs/xfs/xfs_buf_item.h b/fs/xfs/xfs_buf_item.h
> index 30114b510332..c9c57e2da932 100644
> --- a/fs/xfs/xfs_buf_item.h
> +++ b/fs/xfs/xfs_buf_item.h
> @@ -59,8 +59,6 @@ void	xfs_buf_attach_iodone(struct xfs_buf *,
>  			      struct xfs_log_item *);
>  void	xfs_buf_iodone_callbacks(struct xfs_buf *);
>  void	xfs_buf_iodone(struct xfs_buf *, struct xfs_log_item *);
> -bool	xfs_buf_resubmit_failed_buffers(struct xfs_buf *,
> -					struct list_head *);
>  bool	xfs_buf_log_check_iovec(struct xfs_log_iovec *iovec);
>  
>  extern kmem_zone_t	*xfs_buf_item_zone;
> diff --git a/fs/xfs/xfs_dquot_item.c b/fs/xfs/xfs_dquot_item.c
> index baad1748d0d1..5a7808299a32 100644
> --- a/fs/xfs/xfs_dquot_item.c
> +++ b/fs/xfs/xfs_dquot_item.c
> @@ -145,21 +145,6 @@ xfs_qm_dquot_logitem_push(
>  	if (atomic_read(&dqp->q_pincount) > 0)
>  		return XFS_ITEM_PINNED;
>  
> -	/*
> -	 * The buffer containing this item failed to be written back
> -	 * previously. Resubmit the buffer for IO
> -	 */
> -	if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
> -		if (!xfs_buf_trylock(bp))
> -			return XFS_ITEM_LOCKED;
> -
> -		if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list))
> -			rval = XFS_ITEM_FLUSHING;
> -
> -		xfs_buf_unlock(bp);
> -		return rval;
> -	}
> -
>  	if (!xfs_dqlock_nowait(dqp))
>  		return XFS_ITEM_LOCKED;
>  
> diff --git a/fs/xfs/xfs_inode_item.c b/fs/xfs/xfs_inode_item.c
> index f779cca2346f..1d4d256a2e96 100644
> --- a/fs/xfs/xfs_inode_item.c
> +++ b/fs/xfs/xfs_inode_item.c
> @@ -497,21 +497,6 @@ xfs_inode_item_push(
>  	if (xfs_ipincount(ip) > 0)
>  		return XFS_ITEM_PINNED;
>  
> -	/*
> -	 * The buffer containing this item failed to be written back
> -	 * previously. Resubmit the buffer for IO.
> -	 */
> -	if (test_bit(XFS_LI_FAILED, &lip->li_flags)) {
> -		if (!xfs_buf_trylock(bp))
> -			return XFS_ITEM_LOCKED;
> -
> -		if (!xfs_buf_resubmit_failed_buffers(bp, buffer_list))
> -			rval = XFS_ITEM_FLUSHING;
> -
> -		xfs_buf_unlock(bp);
> -		return rval;
> -	}
> -
>  	if (!xfs_ilock_nowait(ip, XFS_ILOCK_SHARED))
>  		return XFS_ITEM_LOCKED;
>  
> diff --git a/fs/xfs/xfs_trans_ail.c b/fs/xfs/xfs_trans_ail.c
> index 564253550b75..2574d01e4a83 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_resubmit_item(
> +	struct xfs_log_item	*lip,
> +	struct list_head	*buffer_list)
> +{
> +	struct xfs_buf		*bp = lip->li_buf;
> +
> +	if (!xfs_buf_trylock(bp))
> +		return XFS_ITEM_LOCKED;
> +
> +	if (!xfs_buf_delwri_queue(bp, buffer_list)) {
> +		xfs_buf_unlock(bp);
> +		return XFS_ITEM_FLUSHING;
> +	}
> +
> +	/* protected by ail_lock */
> +	list_for_each_entry(lip, &bp->b_li_list, li_bio_list)
> +		xfs_clear_li_failed(lip);
> +
> +	xfs_buf_unlock(bp);
> +	return XFS_ITEM_SUCCESS;
> +}
> +
>  static inline uint
>  xfsaild_push_item(
>  	struct xfs_ail		*ailp,
> @@ -365,6 +404,8 @@ xfsaild_push_item(
>  	 */
>  	if (!lip->li_ops->iop_push)
>  		return XFS_ITEM_PINNED;
> +	if (test_bit(XFS_LI_FAILED, &lip->li_flags))
> +		return xfsaild_resubmit_item(lip, &ailp->ail_buf_list);
>  	return lip->li_ops->iop_push(lip, &ailp->ail_buf_list);
>  }
>  
> -- 
> 2.21.1
>