diff mbox series

[3/8] xfs: move and rename xfs_blkdev_issue_flush

Message ID 20210223033442.3267258-4-david@fromorbit.com (mailing list archive)
State New
Headers show
Series [1/8] xfs: log stripe roundoff is a property of the log | expand

Commit Message

Dave Chinner Feb. 23, 2021, 3:34 a.m. UTC
From: Dave Chinner <dchinner@redhat.com>

Move it to xfs_bio_io.c as we are about to add new async cache flush
functionality that uses bios directly, so all this stuff should be
in the same place. Rename the function to xfs_flush_bdev() to match
the xfs_rw_bdev() function that already exists in this file.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_bio_io.c | 8 ++++++++
 fs/xfs/xfs_buf.c    | 2 +-
 fs/xfs/xfs_file.c   | 6 +++---
 fs/xfs/xfs_linux.h  | 1 +
 fs/xfs/xfs_log.c    | 2 +-
 fs/xfs/xfs_super.c  | 7 -------
 fs/xfs/xfs_super.h  | 1 -
 7 files changed, 14 insertions(+), 13 deletions(-)

Comments

Chandan Babu R Feb. 23, 2021, 12:57 p.m. UTC | #1
On 23 Feb 2021 at 09:04, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> Move it to xfs_bio_io.c as we are about to add new async cache flush
> functionality that uses bios directly, so all this stuff should be
> in the same place. Rename the function to xfs_flush_bdev() to match
> the xfs_rw_bdev() function that already exists in this file.
>

Looks good to me.

Reviewed-by: Chandan Babu R <chandanrlinux@gmail.com>

> Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ---
>  fs/xfs/xfs_bio_io.c | 8 ++++++++
>  fs/xfs/xfs_buf.c    | 2 +-
>  fs/xfs/xfs_file.c   | 6 +++---
>  fs/xfs/xfs_linux.h  | 1 +
>  fs/xfs/xfs_log.c    | 2 +-
>  fs/xfs/xfs_super.c  | 7 -------
>  fs/xfs/xfs_super.h  | 1 -
>  7 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c
> index e2148f2d5d6b..5abf653a45d4 100644
> --- a/fs/xfs/xfs_bio_io.c
> +++ b/fs/xfs/xfs_bio_io.c
> @@ -59,3 +59,11 @@ xfs_rw_bdev(
>  		invalidate_kernel_vmap_range(data, count);
>  	return error;
>  }
> +
> +void
> +xfs_flush_bdev(
> +	struct block_device	*bdev)
> +{
> +	blkdev_issue_flush(bdev, GFP_NOFS);
> +}
> +
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index f6e5235df7c9..b1d6c530c693 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1958,7 +1958,7 @@ xfs_free_buftarg(
>  	percpu_counter_destroy(&btp->bt_io_count);
>  	list_lru_destroy(&btp->bt_lru);
>  
> -	xfs_blkdev_issue_flush(btp);
> +	xfs_flush_bdev(btp->bt_bdev);
>  
>  	kmem_free(btp);
>  }
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 38528e59030e..dd33ef2d0e20 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -196,9 +196,9 @@ xfs_file_fsync(
>  	 * inode size in case of an extending write.
>  	 */
>  	if (XFS_IS_REALTIME_INODE(ip))
> -		xfs_blkdev_issue_flush(mp->m_rtdev_targp);
> +		xfs_flush_bdev(mp->m_rtdev_targp->bt_bdev);
>  	else if (mp->m_logdev_targp != mp->m_ddev_targp)
> -		xfs_blkdev_issue_flush(mp->m_ddev_targp);
> +		xfs_flush_bdev(mp->m_ddev_targp->bt_bdev);
>  
>  	/*
>  	 * Any inode that has dirty modifications in the log is pinned.  The
> @@ -218,7 +218,7 @@ xfs_file_fsync(
>  	 */
>  	if (!log_flushed && !XFS_IS_REALTIME_INODE(ip) &&
>  	    mp->m_logdev_targp == mp->m_ddev_targp)
> -		xfs_blkdev_issue_flush(mp->m_ddev_targp);
> +		xfs_flush_bdev(mp->m_ddev_targp->bt_bdev);
>  
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index af6be9b9ccdf..e94a2aeefee8 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -196,6 +196,7 @@ static inline uint64_t howmany_64(uint64_t x, uint32_t y)
>  
>  int xfs_rw_bdev(struct block_device *bdev, sector_t sector, unsigned int count,
>  		char *data, unsigned int op);
> +void xfs_flush_bdev(struct block_device *bdev);
>  
>  #define ASSERT_ALWAYS(expr)	\
>  	(likely(expr) ? (void)0 : assfail(NULL, #expr, __FILE__, __LINE__))
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index ff26fb46d70f..493454c98c6f 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2015,7 +2015,7 @@ xlog_sync(
>  	 * layer state machine for preflushes.
>  	 */
>  	if (log->l_targ != log->l_mp->m_ddev_targp || split) {
> -		xfs_blkdev_issue_flush(log->l_mp->m_ddev_targp);
> +		xfs_flush_bdev(log->l_mp->m_ddev_targp->bt_bdev);
>  		need_flush = false;
>  	}
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 21b1d034aca3..85dd9593b40b 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -339,13 +339,6 @@ xfs_blkdev_put(
>  		blkdev_put(bdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
>  }
>  
> -void
> -xfs_blkdev_issue_flush(
> -	xfs_buftarg_t		*buftarg)
> -{
> -	blkdev_issue_flush(buftarg->bt_bdev, GFP_NOFS);
> -}
> -
>  STATIC void
>  xfs_close_devices(
>  	struct xfs_mount	*mp)
> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> index 1ca484b8357f..79cb2dece811 100644
> --- a/fs/xfs/xfs_super.h
> +++ b/fs/xfs/xfs_super.h
> @@ -88,7 +88,6 @@ struct block_device;
>  
>  extern void xfs_quiesce_attr(struct xfs_mount *mp);
>  extern void xfs_flush_inodes(struct xfs_mount *mp);
> -extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
>  extern xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *,
>  					   xfs_agnumber_t agcount);
Darrick J. Wong Feb. 24, 2021, 8:45 p.m. UTC | #2
On Tue, Feb 23, 2021 at 02:34:37PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Move it to xfs_bio_io.c as we are about to add new async cache flush
> functionality that uses bios directly, so all this stuff should be
> in the same place. Rename the function to xfs_flush_bdev() to match
> the xfs_rw_bdev() function that already exists in this file.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

I don't get why it's necessary to consolidate the synchronous flush
function with the (future) async flush, since all the sync flush callers
go through a buftarg, including the log.  All this seems to do is shift
pointer dereferencing into callers.

Why not make the async flush function take a buftarg?

--D

> ---
>  fs/xfs/xfs_bio_io.c | 8 ++++++++
>  fs/xfs/xfs_buf.c    | 2 +-
>  fs/xfs/xfs_file.c   | 6 +++---
>  fs/xfs/xfs_linux.h  | 1 +
>  fs/xfs/xfs_log.c    | 2 +-
>  fs/xfs/xfs_super.c  | 7 -------
>  fs/xfs/xfs_super.h  | 1 -
>  7 files changed, 14 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c
> index e2148f2d5d6b..5abf653a45d4 100644
> --- a/fs/xfs/xfs_bio_io.c
> +++ b/fs/xfs/xfs_bio_io.c
> @@ -59,3 +59,11 @@ xfs_rw_bdev(
>  		invalidate_kernel_vmap_range(data, count);
>  	return error;
>  }
> +
> +void
> +xfs_flush_bdev(
> +	struct block_device	*bdev)
> +{
> +	blkdev_issue_flush(bdev, GFP_NOFS);
> +}
> +
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index f6e5235df7c9..b1d6c530c693 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1958,7 +1958,7 @@ xfs_free_buftarg(
>  	percpu_counter_destroy(&btp->bt_io_count);
>  	list_lru_destroy(&btp->bt_lru);
>  
> -	xfs_blkdev_issue_flush(btp);
> +	xfs_flush_bdev(btp->bt_bdev);
>  
>  	kmem_free(btp);
>  }
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 38528e59030e..dd33ef2d0e20 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -196,9 +196,9 @@ xfs_file_fsync(
>  	 * inode size in case of an extending write.
>  	 */
>  	if (XFS_IS_REALTIME_INODE(ip))
> -		xfs_blkdev_issue_flush(mp->m_rtdev_targp);
> +		xfs_flush_bdev(mp->m_rtdev_targp->bt_bdev);
>  	else if (mp->m_logdev_targp != mp->m_ddev_targp)
> -		xfs_blkdev_issue_flush(mp->m_ddev_targp);
> +		xfs_flush_bdev(mp->m_ddev_targp->bt_bdev);
>  
>  	/*
>  	 * Any inode that has dirty modifications in the log is pinned.  The
> @@ -218,7 +218,7 @@ xfs_file_fsync(
>  	 */
>  	if (!log_flushed && !XFS_IS_REALTIME_INODE(ip) &&
>  	    mp->m_logdev_targp == mp->m_ddev_targp)
> -		xfs_blkdev_issue_flush(mp->m_ddev_targp);
> +		xfs_flush_bdev(mp->m_ddev_targp->bt_bdev);
>  
>  	return error;
>  }
> diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
> index af6be9b9ccdf..e94a2aeefee8 100644
> --- a/fs/xfs/xfs_linux.h
> +++ b/fs/xfs/xfs_linux.h
> @@ -196,6 +196,7 @@ static inline uint64_t howmany_64(uint64_t x, uint32_t y)
>  
>  int xfs_rw_bdev(struct block_device *bdev, sector_t sector, unsigned int count,
>  		char *data, unsigned int op);
> +void xfs_flush_bdev(struct block_device *bdev);
>  
>  #define ASSERT_ALWAYS(expr)	\
>  	(likely(expr) ? (void)0 : assfail(NULL, #expr, __FILE__, __LINE__))
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index ff26fb46d70f..493454c98c6f 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -2015,7 +2015,7 @@ xlog_sync(
>  	 * layer state machine for preflushes.
>  	 */
>  	if (log->l_targ != log->l_mp->m_ddev_targp || split) {
> -		xfs_blkdev_issue_flush(log->l_mp->m_ddev_targp);
> +		xfs_flush_bdev(log->l_mp->m_ddev_targp->bt_bdev);
>  		need_flush = false;
>  	}
>  
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 21b1d034aca3..85dd9593b40b 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -339,13 +339,6 @@ xfs_blkdev_put(
>  		blkdev_put(bdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
>  }
>  
> -void
> -xfs_blkdev_issue_flush(
> -	xfs_buftarg_t		*buftarg)
> -{
> -	blkdev_issue_flush(buftarg->bt_bdev, GFP_NOFS);
> -}
> -
>  STATIC void
>  xfs_close_devices(
>  	struct xfs_mount	*mp)
> diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
> index 1ca484b8357f..79cb2dece811 100644
> --- a/fs/xfs/xfs_super.h
> +++ b/fs/xfs/xfs_super.h
> @@ -88,7 +88,6 @@ struct block_device;
>  
>  extern void xfs_quiesce_attr(struct xfs_mount *mp);
>  extern void xfs_flush_inodes(struct xfs_mount *mp);
> -extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
>  extern xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *,
>  					   xfs_agnumber_t agcount);
>  
> -- 
> 2.28.0
>
Dave Chinner Feb. 24, 2021, 10:01 p.m. UTC | #3
On Wed, Feb 24, 2021 at 12:45:29PM -0800, Darrick J. Wong wrote:
> On Tue, Feb 23, 2021 at 02:34:37PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@redhat.com>
> > 
> > Move it to xfs_bio_io.c as we are about to add new async cache flush
> > functionality that uses bios directly, so all this stuff should be
> > in the same place. Rename the function to xfs_flush_bdev() to match
> > the xfs_rw_bdev() function that already exists in this file.
> > 
> > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> 
> I don't get why it's necessary to consolidate the synchronous flush
> function with the (future) async flush, since all the sync flush callers
> go through a buftarg, including the log.  All this seems to do is shift
> pointer dereferencing into callers.
> 
> Why not make the async flush function take a buftarg?

Because we pretty much got rid of the buffer/buftarg abstraction
from the log IO code completely by going direct to bios. The async
flush goes direct to bios like the rest of the log code does. And
given that xfs_blkdev_issue_flush() is just a one line wrapper
around the blkdev interface, it just seems totally weird to wrap it
differently to other interfaces that go direct to the bios and block
devices.

Part of the problem here is that we've completely screwed up the
separation/abstraction of the log from the xfs_mount and buftargs.
The log just doesn't use buftargs anymore except as a holder of the
log bdev, and the only other interaction it has with them is when
the metadata device cache needs to be invalidated after log recovery
and during unmount.

It just doesn't make sense to me to have bdev flush interfaces that
the log uses hidden behind an abstraction that the rest of the
log subsystem doesn't use...

Cheers,

Dave.
Christoph Hellwig Feb. 25, 2021, 8:36 a.m. UTC | #4
On Tue, Feb 23, 2021 at 02:34:37PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> Move it to xfs_bio_io.c as we are about to add new async cache flush
> functionality that uses bios directly, so all this stuff should be
> in the same place. Rename the function to xfs_flush_bdev() to match
> the xfs_rw_bdev() function that already exists in this file.

I'd rather kill it off.  None that as of the latest Linus tree,
blkdev_issue_flush has also lost the gfp_mask argument.
diff mbox series

Patch

diff --git a/fs/xfs/xfs_bio_io.c b/fs/xfs/xfs_bio_io.c
index e2148f2d5d6b..5abf653a45d4 100644
--- a/fs/xfs/xfs_bio_io.c
+++ b/fs/xfs/xfs_bio_io.c
@@ -59,3 +59,11 @@  xfs_rw_bdev(
 		invalidate_kernel_vmap_range(data, count);
 	return error;
 }
+
+void
+xfs_flush_bdev(
+	struct block_device	*bdev)
+{
+	blkdev_issue_flush(bdev, GFP_NOFS);
+}
+
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index f6e5235df7c9..b1d6c530c693 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1958,7 +1958,7 @@  xfs_free_buftarg(
 	percpu_counter_destroy(&btp->bt_io_count);
 	list_lru_destroy(&btp->bt_lru);
 
-	xfs_blkdev_issue_flush(btp);
+	xfs_flush_bdev(btp->bt_bdev);
 
 	kmem_free(btp);
 }
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 38528e59030e..dd33ef2d0e20 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -196,9 +196,9 @@  xfs_file_fsync(
 	 * inode size in case of an extending write.
 	 */
 	if (XFS_IS_REALTIME_INODE(ip))
-		xfs_blkdev_issue_flush(mp->m_rtdev_targp);
+		xfs_flush_bdev(mp->m_rtdev_targp->bt_bdev);
 	else if (mp->m_logdev_targp != mp->m_ddev_targp)
-		xfs_blkdev_issue_flush(mp->m_ddev_targp);
+		xfs_flush_bdev(mp->m_ddev_targp->bt_bdev);
 
 	/*
 	 * Any inode that has dirty modifications in the log is pinned.  The
@@ -218,7 +218,7 @@  xfs_file_fsync(
 	 */
 	if (!log_flushed && !XFS_IS_REALTIME_INODE(ip) &&
 	    mp->m_logdev_targp == mp->m_ddev_targp)
-		xfs_blkdev_issue_flush(mp->m_ddev_targp);
+		xfs_flush_bdev(mp->m_ddev_targp->bt_bdev);
 
 	return error;
 }
diff --git a/fs/xfs/xfs_linux.h b/fs/xfs/xfs_linux.h
index af6be9b9ccdf..e94a2aeefee8 100644
--- a/fs/xfs/xfs_linux.h
+++ b/fs/xfs/xfs_linux.h
@@ -196,6 +196,7 @@  static inline uint64_t howmany_64(uint64_t x, uint32_t y)
 
 int xfs_rw_bdev(struct block_device *bdev, sector_t sector, unsigned int count,
 		char *data, unsigned int op);
+void xfs_flush_bdev(struct block_device *bdev);
 
 #define ASSERT_ALWAYS(expr)	\
 	(likely(expr) ? (void)0 : assfail(NULL, #expr, __FILE__, __LINE__))
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index ff26fb46d70f..493454c98c6f 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -2015,7 +2015,7 @@  xlog_sync(
 	 * layer state machine for preflushes.
 	 */
 	if (log->l_targ != log->l_mp->m_ddev_targp || split) {
-		xfs_blkdev_issue_flush(log->l_mp->m_ddev_targp);
+		xfs_flush_bdev(log->l_mp->m_ddev_targp->bt_bdev);
 		need_flush = false;
 	}
 
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 21b1d034aca3..85dd9593b40b 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -339,13 +339,6 @@  xfs_blkdev_put(
 		blkdev_put(bdev, FMODE_READ|FMODE_WRITE|FMODE_EXCL);
 }
 
-void
-xfs_blkdev_issue_flush(
-	xfs_buftarg_t		*buftarg)
-{
-	blkdev_issue_flush(buftarg->bt_bdev, GFP_NOFS);
-}
-
 STATIC void
 xfs_close_devices(
 	struct xfs_mount	*mp)
diff --git a/fs/xfs/xfs_super.h b/fs/xfs/xfs_super.h
index 1ca484b8357f..79cb2dece811 100644
--- a/fs/xfs/xfs_super.h
+++ b/fs/xfs/xfs_super.h
@@ -88,7 +88,6 @@  struct block_device;
 
 extern void xfs_quiesce_attr(struct xfs_mount *mp);
 extern void xfs_flush_inodes(struct xfs_mount *mp);
-extern void xfs_blkdev_issue_flush(struct xfs_buftarg *);
 extern xfs_agnumber_t xfs_set_inode_alloc(struct xfs_mount *,
 					   xfs_agnumber_t agcount);