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