Re: [PATCH 06/15] xfs: remove xfs_buf_delwri_submit_buffers

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jan 06, 2025 at 10:54:43AM +0100, Christoph Hellwig wrote:
> xfs_buf_delwri_submit_buffers has two callers for synchronous and
> asynchronous writes that share very little logic.  Split out a helper for
> the shared per-buffer loop and otherwise open code the submission in the
> two callers.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_buf.c | 121 +++++++++++++++++++++--------------------------
>  1 file changed, 55 insertions(+), 66 deletions(-)

Sheesh, splitting a function into two reduces line count by 11??
Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>

--D

> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 7edd7a1e9dae..e48d796c786b 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -2259,72 +2259,26 @@ xfs_buf_cmp(
>  	return 0;
>  }
>  
> -/*
> - * Submit buffers for write. If wait_list is specified, the buffers are
> - * submitted using sync I/O and placed on the wait list such that the caller can
> - * iowait each buffer. Otherwise async I/O is used and the buffers are released
> - * at I/O completion time. In either case, buffers remain locked until I/O
> - * completes and the buffer is released from the queue.
> - */
> -static int
> -xfs_buf_delwri_submit_buffers(
> -	struct list_head	*buffer_list,
> -	struct list_head	*wait_list)
> +static bool
> +xfs_buf_delwri_submit_prep(
> +	struct xfs_buf		*bp)
>  {
> -	struct xfs_buf		*bp, *n;
> -	int			pinned = 0;
> -	struct blk_plug		plug;
> -
> -	list_sort(NULL, buffer_list, xfs_buf_cmp);
> -
> -	blk_start_plug(&plug);
> -	list_for_each_entry_safe(bp, n, buffer_list, b_list) {
> -		if (!wait_list) {
> -			if (!xfs_buf_trylock(bp))
> -				continue;
> -			if (xfs_buf_ispinned(bp)) {
> -				xfs_buf_unlock(bp);
> -				pinned++;
> -				continue;
> -			}
> -		} else {
> -			xfs_buf_lock(bp);
> -		}
> -
> -		/*
> -		 * Someone else might have written the buffer synchronously or
> -		 * marked it stale in the meantime.  In that case only the
> -		 * _XBF_DELWRI_Q flag got cleared, and we have to drop the
> -		 * reference and remove it from the list here.
> -		 */
> -		if (!(bp->b_flags & _XBF_DELWRI_Q)) {
> -			xfs_buf_list_del(bp);
> -			xfs_buf_relse(bp);
> -			continue;
> -		}
> -
> -		trace_xfs_buf_delwri_split(bp, _RET_IP_);
> -
> -		/*
> -		 * If we have a wait list, each buffer (and associated delwri
> -		 * queue reference) transfers to it and is submitted
> -		 * synchronously. Otherwise, drop the buffer from the delwri
> -		 * queue and submit async.
> -		 */
> -		bp->b_flags &= ~_XBF_DELWRI_Q;
> -		bp->b_flags |= XBF_WRITE;
> -		if (wait_list) {
> -			bp->b_flags &= ~XBF_ASYNC;
> -			list_move_tail(&bp->b_list, wait_list);
> -		} else {
> -			bp->b_flags |= XBF_ASYNC;
> -			xfs_buf_list_del(bp);
> -		}
> -		xfs_buf_submit(bp);
> +	/*
> +	 * Someone else might have written the buffer synchronously or marked it
> +	 * stale in the meantime.  In that case only the _XBF_DELWRI_Q flag got
> +	 * cleared, and we have to drop the reference and remove it from the
> +	 * list here.
> +	 */
> +	if (!(bp->b_flags & _XBF_DELWRI_Q)) {
> +		xfs_buf_list_del(bp);
> +		xfs_buf_relse(bp);
> +		return false;
>  	}
> -	blk_finish_plug(&plug);
>  
> -	return pinned;
> +	trace_xfs_buf_delwri_split(bp, _RET_IP_);
> +	bp->b_flags &= ~_XBF_DELWRI_Q;
> +	bp->b_flags |= XBF_WRITE;
> +	return true;
>  }
>  
>  /*
> @@ -2347,7 +2301,30 @@ int
>  xfs_buf_delwri_submit_nowait(
>  	struct list_head	*buffer_list)
>  {
> -	return xfs_buf_delwri_submit_buffers(buffer_list, NULL);
> +	struct xfs_buf		*bp, *n;
> +	int			pinned = 0;
> +	struct blk_plug		plug;
> +
> +	list_sort(NULL, buffer_list, xfs_buf_cmp);
> +
> +	blk_start_plug(&plug);
> +	list_for_each_entry_safe(bp, n, buffer_list, b_list) {
> +		if (!xfs_buf_trylock(bp))
> +			continue;
> +		if (xfs_buf_ispinned(bp)) {
> +			xfs_buf_unlock(bp);
> +			pinned++;
> +			continue;
> +		}
> +		if (!xfs_buf_delwri_submit_prep(bp))
> +			continue;
> +		bp->b_flags |= XBF_ASYNC;
> +		xfs_buf_list_del(bp);
> +		xfs_buf_submit(bp);
> +	}
> +	blk_finish_plug(&plug);
> +
> +	return pinned;
>  }
>  
>  /*
> @@ -2364,9 +2341,21 @@ xfs_buf_delwri_submit(
>  {
>  	LIST_HEAD		(wait_list);
>  	int			error = 0, error2;
> -	struct xfs_buf		*bp;
> +	struct xfs_buf		*bp, *n;
> +	struct blk_plug		plug;
>  
> -	xfs_buf_delwri_submit_buffers(buffer_list, &wait_list);
> +	list_sort(NULL, buffer_list, xfs_buf_cmp);
> +
> +	blk_start_plug(&plug);
> +	list_for_each_entry_safe(bp, n, buffer_list, b_list) {
> +		xfs_buf_lock(bp);
> +		if (!xfs_buf_delwri_submit_prep(bp))
> +			continue;
> +		bp->b_flags &= ~XBF_ASYNC;
> +		list_move_tail(&bp->b_list, &wait_list);
> +		xfs_buf_submit(bp);
> +	}
> +	blk_finish_plug(&plug);
>  
>  	/* Wait for IO to complete. */
>  	while (!list_empty(&wait_list)) {
> -- 
> 2.45.2
> 
>