| Message ID | 20230808161600.1099516-7-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/13] xfs: reformat the xfs_fs_free prototype | expand |
On Tue, Aug 08, 2023 at 09:15:53AM -0700, Christoph Hellwig wrote: > Closing the block devices logically belongs into xfs_free_buftarg, So instead > of open coding it in the caller move it there and add a check for the s_bdev > so that the main device isn't close as that's done by the VFS helper. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/xfs/xfs_buf.c | 5 +++++ > fs/xfs/xfs_super.c | 12 ++---------- > 2 files changed, 7 insertions(+), 10 deletions(-) > > diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c > index 83b8702030f71d..c57e6e03dfa80c 100644 > --- a/fs/xfs/xfs_buf.c > +++ b/fs/xfs/xfs_buf.c > @@ -1938,6 +1938,8 @@ void > xfs_free_buftarg( > struct xfs_buftarg *btp) > { > + struct block_device *bdev = btp->bt_bdev; > + > unregister_shrinker(&btp->bt_shrinker); > ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0); > percpu_counter_destroy(&btp->bt_io_count); > @@ -1945,6 +1947,9 @@ xfs_free_buftarg( > > blkdev_issue_flush(btp->bt_bdev); > fs_put_dax(btp->bt_daxdev, btp->bt_mount); > + /* the main block device is closed by kill_block_super */ > + if (bdev != btp->bt_mount->m_super->s_bdev) > + blkdev_put(bdev, btp->bt_mount->m_super); Hmm... I feel like this would be cleaner if the data dev buftarg could get its own refcount separate from super_block.s_bdev, but I looked through the code and couldn't identify a simple way to do that. Soo... Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > > kmem_free(btp); > } > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index f00d1162815d19..37b1b763a0bef0 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -399,18 +399,10 @@ STATIC void > xfs_close_devices( > struct xfs_mount *mp) > { > - if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) { > - struct block_device *logdev = mp->m_logdev_targp->bt_bdev; > - > + if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) > xfs_free_buftarg(mp->m_logdev_targp); > - blkdev_put(logdev, mp->m_super); > - } > - if (mp->m_rtdev_targp) { > - struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev; > - > + if (mp->m_rtdev_targp) > xfs_free_buftarg(mp->m_rtdev_targp); > - blkdev_put(rtdev, mp->m_super); > - } > xfs_free_buftarg(mp->m_ddev_targp); > } > > -- > 2.39.2 >
On Wed, Aug 09, 2023 at 08:45:32AM -0700, Darrick J. Wong wrote: > > blkdev_issue_flush(btp->bt_bdev); > > fs_put_dax(btp->bt_daxdev, btp->bt_mount); > > + /* the main block device is closed by kill_block_super */ > > + if (bdev != btp->bt_mount->m_super->s_bdev) > > + blkdev_put(bdev, btp->bt_mount->m_super); > > Hmm... I feel like this would be cleaner if the data dev buftarg could > get its own refcount separate from super_block.s_bdev, but I looked > through the code and couldn't identify a simple way to do that. Soo... blkdev_put doesn't really drop a refcount, it closes the device. It just happens to be misnamed, but Jan is looking into a series that will as a side effect end up with a better name for this functionality.
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c index 83b8702030f71d..c57e6e03dfa80c 100644 --- a/fs/xfs/xfs_buf.c +++ b/fs/xfs/xfs_buf.c @@ -1938,6 +1938,8 @@ void xfs_free_buftarg( struct xfs_buftarg *btp) { + struct block_device *bdev = btp->bt_bdev; + unregister_shrinker(&btp->bt_shrinker); ASSERT(percpu_counter_sum(&btp->bt_io_count) == 0); percpu_counter_destroy(&btp->bt_io_count); @@ -1945,6 +1947,9 @@ xfs_free_buftarg( blkdev_issue_flush(btp->bt_bdev); fs_put_dax(btp->bt_daxdev, btp->bt_mount); + /* the main block device is closed by kill_block_super */ + if (bdev != btp->bt_mount->m_super->s_bdev) + blkdev_put(bdev, btp->bt_mount->m_super); kmem_free(btp); } diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index f00d1162815d19..37b1b763a0bef0 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -399,18 +399,10 @@ STATIC void xfs_close_devices( struct xfs_mount *mp) { - if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) { - struct block_device *logdev = mp->m_logdev_targp->bt_bdev; - + if (mp->m_logdev_targp && mp->m_logdev_targp != mp->m_ddev_targp) xfs_free_buftarg(mp->m_logdev_targp); - blkdev_put(logdev, mp->m_super); - } - if (mp->m_rtdev_targp) { - struct block_device *rtdev = mp->m_rtdev_targp->bt_bdev; - + if (mp->m_rtdev_targp) xfs_free_buftarg(mp->m_rtdev_targp); - blkdev_put(rtdev, mp->m_super); - } xfs_free_buftarg(mp->m_ddev_targp); }
Closing the block devices logically belongs into xfs_free_buftarg, So instead of open coding it in the caller move it there and add a check for the s_bdev so that the main device isn't close as that's done by the VFS helper. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/xfs/xfs_buf.c | 5 +++++ fs/xfs/xfs_super.c | 12 ++---------- 2 files changed, 7 insertions(+), 10 deletions(-)