[PATCH 0/3] Minor cleanups

[PATCH 0/3] Minor cleanups

[Nikolay Borisov]

While digging around xfs' buf code I spotted a couple of cleanup candidates
which resulted in this patch. It should hopefully make the code easier to inspect
by reducing the 'hops' to the actual implementation of buffer submission. 

Nikolay Borisov (3):
  xfs: Use __xfs_buf_submit everywhere
  xfs: Rename __xfs_buf_submit to xfs_buf_submit
  xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP

 fs/xfs/xfs_buf.c         | 16 +++++++++-------
 fs/xfs/xfs_buf.h         | 16 ++++------------
 fs/xfs/xfs_buf_item.c    |  2 +-
 fs/xfs/xfs_log_recover.c |  2 +-
 fs/xfs/xfs_trans.h       |  6 ++++--
 5 files changed, 19 insertions(+), 23 deletions(-)

-- 
2.17.1

[PATCH 1/3] xfs: Use __xfs_buf_submit everywhere

[Nikolay Borisov]

Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit.
It only checks whether XFB_ASYNC flag is set and sets the second
parameter to __xfs_buf_submit accordingly. It's possible to remove the
level of indirection since in all contexts where xfs_buf_submit is
called we already know if XBF_ASYNC is set or not.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/xfs/xfs_buf.c         | 8 +++++---
 fs/xfs/xfs_buf_item.c    | 2 +-
 fs/xfs/xfs_log_recover.c | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index ca0849043f54..a75d05e49a98 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -751,13 +751,15 @@ _xfs_buf_read(
 	xfs_buf_t		*bp,
 	xfs_buf_flags_t		flags)
 {
+	bool wait = bp->b_flags & XBF_ASYNC ? false : true;
+
 	ASSERT(!(flags & XBF_WRITE));
 	ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
 
 	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD);
 	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
 
-	return xfs_buf_submit(bp);
+	return __xfs_buf_submit(bp, wait);
 }
 
 /*
@@ -883,7 +885,7 @@ xfs_buf_read_uncached(
 	bp->b_flags |= XBF_READ;
 	bp->b_ops = ops;
 
-	xfs_buf_submit(bp);
+	__xfs_buf_submit(bp, true);
 	if (bp->b_error) {
 		int	error = bp->b_error;
 		xfs_buf_relse(bp);
@@ -1214,7 +1216,7 @@ xfs_bwrite(
 	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
 			 XBF_WRITE_FAIL | XBF_DONE);
 
-	error = xfs_buf_submit(bp);
+	error = __xfs_buf_submit(bp, true);
 	if (error)
 		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
 	return error;
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index 7dcaec54a20b..fef08980dd21 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error(
 			bp->b_first_retry_time = jiffies;
 
 		xfs_buf_ioerror(bp, 0);
-		xfs_buf_submit(bp);
+		__xfs_buf_submit(bp, false);
 		return true;
 	}
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 13d1d3e95b88..64e315f80147 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -5610,7 +5610,7 @@ xlog_do_recover(
 	bp->b_flags |= XBF_READ;
 	bp->b_ops = &xfs_sb_buf_ops;
 
-	error = xfs_buf_submit(bp);
+	error = __xfs_buf_submit(bp, true);
 	if (error) {
 		if (!XFS_FORCED_SHUTDOWN(mp)) {
 			xfs_buf_ioerror_alert(bp, __func__);
-- 
2.17.1

[PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit

[Nikolay Borisov]

Since xfs_buf_submit no longer has any callers just rename its __
prefixed counterpart.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/xfs/xfs_buf.c         | 10 +++++-----
 fs/xfs/xfs_buf.h         |  7 +------
 fs/xfs/xfs_buf_item.c    |  2 +-
 fs/xfs/xfs_log_recover.c |  2 +-
 4 files changed, 8 insertions(+), 13 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index a75d05e49a98..99c66f80d7cc 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -759,7 +759,7 @@ _xfs_buf_read(
 	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD);
 	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
 
-	return __xfs_buf_submit(bp, wait);
+	return xfs_buf_submit(bp, wait);
 }
 
 /*
@@ -885,7 +885,7 @@ xfs_buf_read_uncached(
 	bp->b_flags |= XBF_READ;
 	bp->b_ops = ops;
 
-	__xfs_buf_submit(bp, true);
+	xfs_buf_submit(bp, true);
 	if (bp->b_error) {
 		int	error = bp->b_error;
 		xfs_buf_relse(bp);
@@ -1216,7 +1216,7 @@ xfs_bwrite(
 	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
 			 XBF_WRITE_FAIL | XBF_DONE);
 
-	error = __xfs_buf_submit(bp, true);
+	error = xfs_buf_submit(bp, true);
 	if (error)
 		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
 	return error;
@@ -1427,7 +1427,7 @@ xfs_buf_iowait(
  * holds an additional reference itself.
  */
 int
-__xfs_buf_submit(
+xfs_buf_submit(
 	struct xfs_buf	*bp,
 	bool		wait)
 {
@@ -1929,7 +1929,7 @@ xfs_buf_delwri_submit_buffers(
 			bp->b_flags |= XBF_ASYNC;
 			list_del_init(&bp->b_list);
 		}
-		__xfs_buf_submit(bp, false);
+		xfs_buf_submit(bp, false);
 	}
 	blk_finish_plug(&plug);
 
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index c6e57a3f409e..ec7037284d62 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -262,12 +262,7 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error,
 #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address)
 extern void xfs_buf_ioerror_alert(struct xfs_buf *, const char *func);
 
-extern int __xfs_buf_submit(struct xfs_buf *bp, bool);
-static inline int xfs_buf_submit(struct xfs_buf *bp)
-{
-	bool wait = bp->b_flags & XBF_ASYNC ? false : true;
-	return __xfs_buf_submit(bp, wait);
-}
+extern int xfs_buf_submit(struct xfs_buf *bp, bool);
 
 void xfs_buf_zero(struct xfs_buf *bp, size_t boff, size_t bsize);
 
diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
index fef08980dd21..93f38fdceb80 100644
--- a/fs/xfs/xfs_buf_item.c
+++ b/fs/xfs/xfs_buf_item.c
@@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error(
 			bp->b_first_retry_time = jiffies;
 
 		xfs_buf_ioerror(bp, 0);
-		__xfs_buf_submit(bp, false);
+		xfs_buf_submit(bp, false);
 		return true;
 	}
 
diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
index 64e315f80147..9b7822638f83 100644
--- a/fs/xfs/xfs_log_recover.c
+++ b/fs/xfs/xfs_log_recover.c
@@ -5610,7 +5610,7 @@ xlog_do_recover(
 	bp->b_flags |= XBF_READ;
 	bp->b_ops = &xfs_sb_buf_ops;
 
-	error = __xfs_buf_submit(bp, true);
+	error = xfs_buf_submit(bp, true);
 	if (error) {
 		if (!XFS_FORCED_SHUTDOWN(mp)) {
 			xfs_buf_ioerror_alert(bp, __func__);
-- 
2.17.1

[PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP

[Nikolay Borisov]

This macro encodes a trivial struct initializations, just open code it.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/xfs/xfs_buf.c   | 4 ++--
 fs/xfs/xfs_buf.h   | 9 +++------
 fs/xfs/xfs_trans.h | 6 ++++--
 3 files changed, 9 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index 99c66f80d7cc..389c5b590f11 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -658,7 +658,7 @@ xfs_buf_incore(
 {
 	struct xfs_buf		*bp;
 	int			error;
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
 
 	error = xfs_buf_find(target, &map, 1, flags, NULL, &bp);
 	if (error)
@@ -905,7 +905,7 @@ xfs_buf_get_uncached(
 	unsigned long		page_count;
 	int			error, i;
 	struct xfs_buf		*bp;
-	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
+	struct xfs_buf_map map = { .bm_bn = XFS_BUF_DADDR_NULL, .bm_len = numblks };
 
 	/* flags might contain irrelevant bits, pass only what we care about */
 	bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT);
diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
index ec7037284d62..548dfb0c6e27 100644
--- a/fs/xfs/xfs_buf.h
+++ b/fs/xfs/xfs_buf.h
@@ -104,9 +104,6 @@ struct xfs_buf_map {
 	int			bm_len;	/* size of I/O */
 };
 
-#define DEFINE_SINGLE_BUF_MAP(map, blkno, numblk) \
-	struct xfs_buf_map (map) = { .bm_bn = (blkno), .bm_len = (numblk) };
-
 struct xfs_buf_ops {
 	char *name;
 	union {
@@ -209,7 +206,7 @@ xfs_buf_get(
 	xfs_daddr_t		blkno,
 	size_t			numblks)
 {
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
 	return xfs_buf_get_map(target, &map, 1, 0);
 }
 
@@ -221,7 +218,7 @@ xfs_buf_read(
 	xfs_buf_flags_t		flags,
 	const struct xfs_buf_ops *ops)
 {
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
 	return xfs_buf_read_map(target, &map, 1, flags, ops);
 }
 
@@ -232,7 +229,7 @@ xfs_buf_readahead(
 	size_t			numblks,
 	const struct xfs_buf_ops *ops)
 {
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
 	return xfs_buf_readahead_map(target, &map, 1, ops);
 }
 
diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
index 64d7f171ebd3..8d6fce5c0320 100644
--- a/fs/xfs/xfs_trans.h
+++ b/fs/xfs/xfs_trans.h
@@ -182,7 +182,8 @@ xfs_trans_get_buf(
 	int			numblks,
 	uint			flags)
 {
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
+
 	return xfs_trans_get_buf_map(tp, target, &map, 1, flags);
 }
 
@@ -205,7 +206,8 @@ xfs_trans_read_buf(
 	struct xfs_buf		**bpp,
 	const struct xfs_buf_ops *ops)
 {
-	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
+	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
+
 	return xfs_trans_read_buf_map(mp, tp, target, &map, 1,
 				      flags, bpp, ops);
 }
-- 
2.17.1

Re: [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere

[Brian Foster]

On Tue, Aug 13, 2019 at 12:03:04PM +0300, Nikolay Borisov wrote:
> Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit.
> It only checks whether XFB_ASYNC flag is set and sets the second
> parameter to __xfs_buf_submit accordingly. It's possible to remove the
> level of indirection since in all contexts where xfs_buf_submit is
> called we already know if XBF_ASYNC is set or not.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---

Random nit: the use of upper case in the first word of the commit log
subject line kind of stands out to me. I know there are other instances
of this (I think I noticed one the other day), but my presumption was
that it was random/accidental where your patches seem to do it
intentionally. Do we have a common practice here? Do we care? I prefer
consistency of using lower case for normal text, but it's really just a
nit.

>  fs/xfs/xfs_buf.c         | 8 +++++---
>  fs/xfs/xfs_buf_item.c    | 2 +-
>  fs/xfs/xfs_log_recover.c | 2 +-
>  3 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index ca0849043f54..a75d05e49a98 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -751,13 +751,15 @@ _xfs_buf_read(
>  	xfs_buf_t		*bp,
>  	xfs_buf_flags_t		flags)
>  {
> +	bool wait = bp->b_flags & XBF_ASYNC ? false : true;
> +

This doesn't look quite right. Just below we clear several flags from
->b_flags then potentially reapply based on the flags parameter. Hence,
I think ->b_flags above may not reflect ->b_flags by the time we call
__xfs_buf_submit().

Brian

>  	ASSERT(!(flags & XBF_WRITE));
>  	ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
>  
>  	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD);
>  	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
>  
> -	return xfs_buf_submit(bp);
> +	return __xfs_buf_submit(bp, wait);
>  }
>  
>  /*
> @@ -883,7 +885,7 @@ xfs_buf_read_uncached(
>  	bp->b_flags |= XBF_READ;
>  	bp->b_ops = ops;
>  
> -	xfs_buf_submit(bp);
> +	__xfs_buf_submit(bp, true);
>  	if (bp->b_error) {
>  		int	error = bp->b_error;
>  		xfs_buf_relse(bp);
> @@ -1214,7 +1216,7 @@ xfs_bwrite(
>  	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
>  			 XBF_WRITE_FAIL | XBF_DONE);
>  
> -	error = xfs_buf_submit(bp);
> +	error = __xfs_buf_submit(bp, true);
>  	if (error)
>  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
>  	return error;
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index 7dcaec54a20b..fef08980dd21 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error(
>  			bp->b_first_retry_time = jiffies;
>  
>  		xfs_buf_ioerror(bp, 0);
> -		xfs_buf_submit(bp);
> +		__xfs_buf_submit(bp, false);
>  		return true;
>  	}
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 13d1d3e95b88..64e315f80147 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -5610,7 +5610,7 @@ xlog_do_recover(
>  	bp->b_flags |= XBF_READ;
>  	bp->b_ops = &xfs_sb_buf_ops;
>  
> -	error = xfs_buf_submit(bp);
> +	error = __xfs_buf_submit(bp, true);
>  	if (error) {
>  		if (!XFS_FORCED_SHUTDOWN(mp)) {
>  			xfs_buf_ioerror_alert(bp, __func__);
> -- 
> 2.17.1
> 

Re: [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit

[Brian Foster]

On Tue, Aug 13, 2019 at 12:03:05PM +0300, Nikolay Borisov wrote:
> Since xfs_buf_submit no longer has any callers just rename its __
> prefixed counterpart.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---

Now we have a primary submission interface that allows combinations of
XBF_ASYNC and waiting or not while the underlying mechanisms are not so
flexible. It looks like the current factoring exists to support delwri
queues where we never wait in buffer submission regardless of async
state because we are batching the submission/wait across multiple
buffers. But what happens if a caller passes an async buffer with wait
== true? I/O completion only completes ->b_iowait if XBF_ASYNC is clear.

I find this rather confusing because now a caller needs to know about
implementation details to use the function properly. That's already true
of __xfs_buf_submit(), but that's partly why it's named as an "internal"
function. I think we ultimately need the interface flexibility so the
delwri case can continue to work. One option could be to update
xfs_buf_submit() such that we never wait on an XBF_ASYNC buffer and add
an assert to flag wait == true as invalid, but TBH I'm not convinced
this is any simpler than the current interface where most callers simply
only need to care about the flag. Maybe others have thoughts...

Brian

>  fs/xfs/xfs_buf.c         | 10 +++++-----
>  fs/xfs/xfs_buf.h         |  7 +------
>  fs/xfs/xfs_buf_item.c    |  2 +-
>  fs/xfs/xfs_log_recover.c |  2 +-
>  4 files changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index a75d05e49a98..99c66f80d7cc 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -759,7 +759,7 @@ _xfs_buf_read(
>  	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD);
>  	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
>  
> -	return __xfs_buf_submit(bp, wait);
> +	return xfs_buf_submit(bp, wait);
>  }
>  
>  /*
> @@ -885,7 +885,7 @@ xfs_buf_read_uncached(
>  	bp->b_flags |= XBF_READ;
>  	bp->b_ops = ops;
>  
> -	__xfs_buf_submit(bp, true);
> +	xfs_buf_submit(bp, true);
>  	if (bp->b_error) {
>  		int	error = bp->b_error;
>  		xfs_buf_relse(bp);
> @@ -1216,7 +1216,7 @@ xfs_bwrite(
>  	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
>  			 XBF_WRITE_FAIL | XBF_DONE);
>  
> -	error = __xfs_buf_submit(bp, true);
> +	error = xfs_buf_submit(bp, true);
>  	if (error)
>  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
>  	return error;
> @@ -1427,7 +1427,7 @@ xfs_buf_iowait(
>   * holds an additional reference itself.
>   */
>  int
> -__xfs_buf_submit(
> +xfs_buf_submit(
>  	struct xfs_buf	*bp,
>  	bool		wait)
>  {
> @@ -1929,7 +1929,7 @@ xfs_buf_delwri_submit_buffers(
>  			bp->b_flags |= XBF_ASYNC;
>  			list_del_init(&bp->b_list);
>  		}
> -		__xfs_buf_submit(bp, false);
> +		xfs_buf_submit(bp, false);
>  	}
>  	blk_finish_plug(&plug);
>  
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index c6e57a3f409e..ec7037284d62 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -262,12 +262,7 @@ extern void __xfs_buf_ioerror(struct xfs_buf *bp, int error,
>  #define xfs_buf_ioerror(bp, err) __xfs_buf_ioerror((bp), (err), __this_address)
>  extern void xfs_buf_ioerror_alert(struct xfs_buf *, const char *func);
>  
> -extern int __xfs_buf_submit(struct xfs_buf *bp, bool);
> -static inline int xfs_buf_submit(struct xfs_buf *bp)
> -{
> -	bool wait = bp->b_flags & XBF_ASYNC ? false : true;
> -	return __xfs_buf_submit(bp, wait);
> -}
> +extern int xfs_buf_submit(struct xfs_buf *bp, bool);
>  
>  void xfs_buf_zero(struct xfs_buf *bp, size_t boff, size_t bsize);
>  
> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
> index fef08980dd21..93f38fdceb80 100644
> --- a/fs/xfs/xfs_buf_item.c
> +++ b/fs/xfs/xfs_buf_item.c
> @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error(
>  			bp->b_first_retry_time = jiffies;
>  
>  		xfs_buf_ioerror(bp, 0);
> -		__xfs_buf_submit(bp, false);
> +		xfs_buf_submit(bp, false);
>  		return true;
>  	}
>  
> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> index 64e315f80147..9b7822638f83 100644
> --- a/fs/xfs/xfs_log_recover.c
> +++ b/fs/xfs/xfs_log_recover.c
> @@ -5610,7 +5610,7 @@ xlog_do_recover(
>  	bp->b_flags |= XBF_READ;
>  	bp->b_ops = &xfs_sb_buf_ops;
>  
> -	error = __xfs_buf_submit(bp, true);
> +	error = xfs_buf_submit(bp, true);
>  	if (error) {
>  		if (!XFS_FORCED_SHUTDOWN(mp)) {
>  			xfs_buf_ioerror_alert(bp, __func__);
> -- 
> 2.17.1
> 

Re: [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP

[Brian Foster]

On Tue, Aug 13, 2019 at 12:03:06PM +0300, Nikolay Borisov wrote:
> This macro encodes a trivial struct initializations, just open code it.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---

Seems fine:

Reviewed-by: Brian Foster <bfoster@redhat.com>

What might be more interesting is to audit the cases where nmap is
always 1 and see if we can start eliminating some of this code where it
isn't needed. For example, it looks xfs_buf_readahead_map() only ever
uses nmap == 1. Can we pass a block/len directly there and push the
map/nmap parameters further down the stack? FWIW, I also see several
functions on a quick glance (xfs_dabuf_map(), xfs_buf_map_from_irec())
that take map/nmaps params, assert that nmaps == 1 yet still have
iteration code for nmap > 1 cases.

>  fs/xfs/xfs_buf.c   | 4 ++--
>  fs/xfs/xfs_buf.h   | 9 +++------
>  fs/xfs/xfs_trans.h | 6 ++++--
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 99c66f80d7cc..389c5b590f11 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -658,7 +658,7 @@ xfs_buf_incore(
>  {
>  	struct xfs_buf		*bp;
>  	int			error;
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
>  
>  	error = xfs_buf_find(target, &map, 1, flags, NULL, &bp);
>  	if (error)
> @@ -905,7 +905,7 @@ xfs_buf_get_uncached(
>  	unsigned long		page_count;
>  	int			error, i;
>  	struct xfs_buf		*bp;
> -	DEFINE_SINGLE_BUF_MAP(map, XFS_BUF_DADDR_NULL, numblks);
> +	struct xfs_buf_map map = { .bm_bn = XFS_BUF_DADDR_NULL, .bm_len = numblks };
>  
>  	/* flags might contain irrelevant bits, pass only what we care about */
>  	bp = _xfs_buf_alloc(target, &map, 1, flags & XBF_NO_IOACCT);
> diff --git a/fs/xfs/xfs_buf.h b/fs/xfs/xfs_buf.h
> index ec7037284d62..548dfb0c6e27 100644
> --- a/fs/xfs/xfs_buf.h
> +++ b/fs/xfs/xfs_buf.h
> @@ -104,9 +104,6 @@ struct xfs_buf_map {
>  	int			bm_len;	/* size of I/O */
>  };
>  
> -#define DEFINE_SINGLE_BUF_MAP(map, blkno, numblk) \
> -	struct xfs_buf_map (map) = { .bm_bn = (blkno), .bm_len = (numblk) };
> -
>  struct xfs_buf_ops {
>  	char *name;
>  	union {
> @@ -209,7 +206,7 @@ xfs_buf_get(
>  	xfs_daddr_t		blkno,
>  	size_t			numblks)
>  {
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
>  	return xfs_buf_get_map(target, &map, 1, 0);
>  }
>  
> @@ -221,7 +218,7 @@ xfs_buf_read(
>  	xfs_buf_flags_t		flags,
>  	const struct xfs_buf_ops *ops)
>  {
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
>  	return xfs_buf_read_map(target, &map, 1, flags, ops);
>  }
>  
> @@ -232,7 +229,7 @@ xfs_buf_readahead(
>  	size_t			numblks,
>  	const struct xfs_buf_ops *ops)
>  {
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
>  	return xfs_buf_readahead_map(target, &map, 1, ops);
>  }
>  
> diff --git a/fs/xfs/xfs_trans.h b/fs/xfs/xfs_trans.h
> index 64d7f171ebd3..8d6fce5c0320 100644
> --- a/fs/xfs/xfs_trans.h
> +++ b/fs/xfs/xfs_trans.h
> @@ -182,7 +182,8 @@ xfs_trans_get_buf(
>  	int			numblks,
>  	uint			flags)
>  {
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
> +
>  	return xfs_trans_get_buf_map(tp, target, &map, 1, flags);
>  }
>  
> @@ -205,7 +206,8 @@ xfs_trans_read_buf(
>  	struct xfs_buf		**bpp,
>  	const struct xfs_buf_ops *ops)
>  {
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };
> +
>  	return xfs_trans_read_buf_map(mp, tp, target, &map, 1,
>  				      flags, bpp, ops);
>  }
> -- 
> 2.17.1
> 

Re: [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere

[Nikolay Borisov]

On 13.08.19 г. 14:55 ч., Brian Foster wrote:
> On Tue, Aug 13, 2019 at 12:03:04PM +0300, Nikolay Borisov wrote:
>> Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit.
>> It only checks whether XFB_ASYNC flag is set and sets the second
>> parameter to __xfs_buf_submit accordingly. It's possible to remove the
>> level of indirection since in all contexts where xfs_buf_submit is
>> called we already know if XBF_ASYNC is set or not.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
> 
> Random nit: the use of upper case in the first word of the commit log
> subject line kind of stands out to me. I know there are other instances
> of this (I think I noticed one the other day), but my presumption was
> that it was random/accidental where your patches seem to do it
> intentionally. Do we have a common practice here? Do we care? I prefer
> consistency of using lower case for normal text, but it's really just a
> nit.

I consider the commit log subject and commit log body to be 2 separate
paragraphs, hence I start each one with capital letter.

> 
>>  fs/xfs/xfs_buf.c         | 8 +++++---
>>  fs/xfs/xfs_buf_item.c    | 2 +-
>>  fs/xfs/xfs_log_recover.c | 2 +-
>>  3 files changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
>> index ca0849043f54..a75d05e49a98 100644
>> --- a/fs/xfs/xfs_buf.c
>> +++ b/fs/xfs/xfs_buf.c
>> @@ -751,13 +751,15 @@ _xfs_buf_read(
>>  	xfs_buf_t		*bp,
>>  	xfs_buf_flags_t		flags)
>>  {
>> +	bool wait = bp->b_flags & XBF_ASYNC ? false : true;
>> +
> 
> This doesn't look quite right. Just below we clear several flags from
> ->b_flags then potentially reapply based on the flags parameter. Hence,
> I think ->b_flags above may not reflect ->b_flags by the time we call
> __xfs_buf_submit().

It's correct the flag clearing/setting ensures that the only flags we
have in bp->b_flags are in the set: flags & (XBF_READ | XBF_ASYNC |
XBF_READ_AHEAD);

So if XBF_ASYNC was set initially it will also be set when we call
xfs_buf_submit.


> 
> Brian
> 
>>  	ASSERT(!(flags & XBF_WRITE));
>>  	ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
>>  
>>  	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD);
>>  	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
>>  
>> -	return xfs_buf_submit(bp);
>> +	return __xfs_buf_submit(bp, wait);
>>  }
>>  
>>  /*
>> @@ -883,7 +885,7 @@ xfs_buf_read_uncached(
>>  	bp->b_flags |= XBF_READ;
>>  	bp->b_ops = ops;
>>  
>> -	xfs_buf_submit(bp);
>> +	__xfs_buf_submit(bp, true);
>>  	if (bp->b_error) {
>>  		int	error = bp->b_error;
>>  		xfs_buf_relse(bp);
>> @@ -1214,7 +1216,7 @@ xfs_bwrite(
>>  	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
>>  			 XBF_WRITE_FAIL | XBF_DONE);
>>  
>> -	error = xfs_buf_submit(bp);
>> +	error = __xfs_buf_submit(bp, true);
>>  	if (error)
>>  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
>>  	return error;
>> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
>> index 7dcaec54a20b..fef08980dd21 100644
>> --- a/fs/xfs/xfs_buf_item.c
>> +++ b/fs/xfs/xfs_buf_item.c
>> @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error(
>>  			bp->b_first_retry_time = jiffies;
>>  
>>  		xfs_buf_ioerror(bp, 0);
>> -		xfs_buf_submit(bp);
>> +		__xfs_buf_submit(bp, false);
>>  		return true;
>>  	}
>>  
>> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
>> index 13d1d3e95b88..64e315f80147 100644
>> --- a/fs/xfs/xfs_log_recover.c
>> +++ b/fs/xfs/xfs_log_recover.c
>> @@ -5610,7 +5610,7 @@ xlog_do_recover(
>>  	bp->b_flags |= XBF_READ;
>>  	bp->b_ops = &xfs_sb_buf_ops;
>>  
>> -	error = xfs_buf_submit(bp);
>> +	error = __xfs_buf_submit(bp, true);
>>  	if (error) {
>>  		if (!XFS_FORCED_SHUTDOWN(mp)) {
>>  			xfs_buf_ioerror_alert(bp, __func__);
>> -- 
>> 2.17.1
>>
> 

Re: [PATCH 1/3] xfs: Use __xfs_buf_submit everywhere

[Nikolay Borisov]

On 13.08.19 г. 15:06 ч., Nikolay Borisov wrote:
> 
> 
> On 13.08.19 г. 14:55 ч., Brian Foster wrote:
>> On Tue, Aug 13, 2019 at 12:03:04PM +0300, Nikolay Borisov wrote:
>>> Currently xfs_buf_submit is used as a tiny wrapper to __xfs_buf_submit.
>>> It only checks whether XFB_ASYNC flag is set and sets the second
>>> parameter to __xfs_buf_submit accordingly. It's possible to remove the
>>> level of indirection since in all contexts where xfs_buf_submit is
>>> called we already know if XBF_ASYNC is set or not.
>>>
>>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>>> ---
>>
>> Random nit: the use of upper case in the first word of the commit log
>> subject line kind of stands out to me. I know there are other instances
>> of this (I think I noticed one the other day), but my presumption was
>> that it was random/accidental where your patches seem to do it
>> intentionally. Do we have a common practice here? Do we care? I prefer
>> consistency of using lower case for normal text, but it's really just a
>> nit.
> 
> I consider the commit log subject and commit log body to be 2 separate
> paragraphs, hence I start each one with capital letter.
> 
>>
>>>  fs/xfs/xfs_buf.c         | 8 +++++---
>>>  fs/xfs/xfs_buf_item.c    | 2 +-
>>>  fs/xfs/xfs_log_recover.c | 2 +-
>>>  3 files changed, 7 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
>>> index ca0849043f54..a75d05e49a98 100644
>>> --- a/fs/xfs/xfs_buf.c
>>> +++ b/fs/xfs/xfs_buf.c
>>> @@ -751,13 +751,15 @@ _xfs_buf_read(
>>>  	xfs_buf_t		*bp,
>>>  	xfs_buf_flags_t		flags)
>>>  {
>>> +	bool wait = bp->b_flags & XBF_ASYNC ? false : true;
>>> +
>>
>> This doesn't look quite right. Just below we clear several flags from
>> ->b_flags then potentially reapply based on the flags parameter. Hence,
>> I think ->b_flags above may not reflect ->b_flags by the time we call
>> __xfs_buf_submit().
> 
> It's correct the flag clearing/setting ensures that the only flags we
> have in bp->b_flags are in the set: flags & (XBF_READ | XBF_ASYNC |
> XBF_READ_AHEAD);
> 
> So if XBF_ASYNC was set initially it will also be set when we call
> xfs_buf_submit.

Ah, I see what you meant, indeed the correct check would be :

flags & XBF_ASYNC ...

I will wait to see if people actually consider this series useful and
then resubmit a fixed version.

> 
> 
>>
>> Brian
>>
>>>  	ASSERT(!(flags & XBF_WRITE));
>>>  	ASSERT(bp->b_maps[0].bm_bn != XFS_BUF_DADDR_NULL);
>>>  
>>>  	bp->b_flags &= ~(XBF_WRITE | XBF_ASYNC | XBF_READ_AHEAD);
>>>  	bp->b_flags |= flags & (XBF_READ | XBF_ASYNC | XBF_READ_AHEAD);
>>>  
>>> -	return xfs_buf_submit(bp);
>>> +	return __xfs_buf_submit(bp, wait);
>>>  }
>>>  
>>>  /*
>>> @@ -883,7 +885,7 @@ xfs_buf_read_uncached(
>>>  	bp->b_flags |= XBF_READ;
>>>  	bp->b_ops = ops;
>>>  
>>> -	xfs_buf_submit(bp);
>>> +	__xfs_buf_submit(bp, true);
>>>  	if (bp->b_error) {
>>>  		int	error = bp->b_error;
>>>  		xfs_buf_relse(bp);
>>> @@ -1214,7 +1216,7 @@ xfs_bwrite(
>>>  	bp->b_flags &= ~(XBF_ASYNC | XBF_READ | _XBF_DELWRI_Q |
>>>  			 XBF_WRITE_FAIL | XBF_DONE);
>>>  
>>> -	error = xfs_buf_submit(bp);
>>> +	error = __xfs_buf_submit(bp, true);
>>>  	if (error)
>>>  		xfs_force_shutdown(bp->b_mount, SHUTDOWN_META_IO_ERROR);
>>>  	return error;
>>> diff --git a/fs/xfs/xfs_buf_item.c b/fs/xfs/xfs_buf_item.c
>>> index 7dcaec54a20b..fef08980dd21 100644
>>> --- a/fs/xfs/xfs_buf_item.c
>>> +++ b/fs/xfs/xfs_buf_item.c
>>> @@ -1123,7 +1123,7 @@ xfs_buf_iodone_callback_error(
>>>  			bp->b_first_retry_time = jiffies;
>>>  
>>>  		xfs_buf_ioerror(bp, 0);
>>> -		xfs_buf_submit(bp);
>>> +		__xfs_buf_submit(bp, false);
>>>  		return true;
>>>  	}
>>>  
>>> diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
>>> index 13d1d3e95b88..64e315f80147 100644
>>> --- a/fs/xfs/xfs_log_recover.c
>>> +++ b/fs/xfs/xfs_log_recover.c
>>> @@ -5610,7 +5610,7 @@ xlog_do_recover(
>>>  	bp->b_flags |= XBF_READ;
>>>  	bp->b_ops = &xfs_sb_buf_ops;
>>>  
>>> -	error = xfs_buf_submit(bp);
>>> +	error = __xfs_buf_submit(bp, true);
>>>  	if (error) {
>>>  		if (!XFS_FORCED_SHUTDOWN(mp)) {
>>>  			xfs_buf_ioerror_alert(bp, __func__);
>>> -- 
>>> 2.17.1
>>>
>>
> 

Re: [PATCH 2/3] xfs: Rename __xfs_buf_submit to xfs_buf_submit

[Dave Chinner]

On Tue, Aug 13, 2019 at 07:56:58AM -0400, Brian Foster wrote:
> On Tue, Aug 13, 2019 at 12:03:05PM +0300, Nikolay Borisov wrote:
> > Since xfs_buf_submit no longer has any callers just rename its __
> > prefixed counterpart.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> 
> Now we have a primary submission interface that allows combinations of
> XBF_ASYNC and waiting or not while the underlying mechanisms are not so
> flexible. It looks like the current factoring exists to support delwri
> queues where we never wait in buffer submission regardless of async
> state because we are batching the submission/wait across multiple
> buffers. But what happens if a caller passes an async buffer with wait
> == true? I/O completion only completes ->b_iowait if XBF_ASYNC is clear.
> 
> I find this rather confusing because now a caller needs to know about
> implementation details to use the function properly. That's already true
> of __xfs_buf_submit(), but that's partly why it's named as an "internal"
> function. I think we ultimately need the interface flexibility so the
> delwri case can continue to work. One option could be to update
> xfs_buf_submit() such that we never wait on an XBF_ASYNC buffer and add
> an assert to flag wait == true as invalid, but TBH I'm not convinced
> this is any simpler than the current interface where most callers simply
> only need to care about the flag. Maybe others have thoughts...

Yeah, we slpit the code u plike this intentionally to separate out
the different ways of submitting IO so that we didn't end up using
invalid methods, like ASYNC + wait, which would lead to hangs
waiting for IO that has already completed.

I much prefer the code as it stands now - it may be slightly more
verbose, but it's simple to understand and hard to use
incorrectly....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 3/3] xfs: Opencode and remove DEFINE_SINGLE_BUF_MAP

[Dave Chinner]

On Tue, Aug 13, 2019 at 12:03:06PM +0300, Nikolay Borisov wrote:
> This macro encodes a trivial struct initializations, just open code it.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>

Hmmmm.

We have defines for this sort of structure definition and
initialisation all over the kernel. e.g. LIST_HEAD(),
DEFINE_PER_CPU(), DEFINE_HASHTABLE(), DEFINE_SPINLOCK(), etc...

And really, the intent of the define was to make it easy to get
rid of all the callers of the non-map buffer interfaces by moving
the map definition into the callers of xfs_buf_get, _read, etc
and then enabling use to remove the non-map interfaces
altogether.

Hence I'd much prefer to see the xfs_buf_{get,read,readahead} and
xfs_trans_buf_{get,read} wrapper functions go away than removing the
define.

> ---
>  fs/xfs/xfs_buf.c   | 4 ++--
>  fs/xfs/xfs_buf.h   | 9 +++------
>  fs/xfs/xfs_trans.h | 6 ++++--
>  3 files changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index 99c66f80d7cc..389c5b590f11 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -658,7 +658,7 @@ xfs_buf_incore(
>  {
>  	struct xfs_buf		*bp;
>  	int			error;
> -	DEFINE_SINGLE_BUF_MAP(map, blkno, numblks);
> +	struct xfs_buf_map map = { .bm_bn = blkno, .bm_len = numblks };

FWIW, I'm not a fan of single line definitions like this because
they are really hard to read. If you are converting to this form, it
should be like this:

	struct xfs_buf_map map = {
		.bm_bn = blkno,
		.bm_len = numblks,
	};

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com