[PATCH v2 0/4] Add invalidate_disk() helper for drivers to invalidate the gendisk

This series comes from Christoph Hellwig's suggestion [1]. Some block
device drivers such as loop driver and nbd driver need to invalidate
the gendisk when the backend is detached so that the gendisk can be
reused by the new backend. Now the invalidation is done in device
driver with their own ways. To avoid code duplication and hide
some internals of the implementation, this series adds a block layer
helper and makes both loop driver and nbd driver use it.

[1] https://lore.kernel.org/all/YTmqJHd7YWAQ2lZ7@infradead.org/

V1 to V2:
- Rename invalidate_gendisk() to invalidate_disk()
- Add a cleanup patch to remove bdev checks and bdev variable in __loop_clr_fd()

Xie Yongji (4):
  block: Add invalidate_disk() helper to invalidate the gendisk
  loop: Use invalidate_disk() helper to invalidate gendisk
  loop: Remove the unnecessary bdev checks and unused bdev variable
  nbd: Use invalidate_disk() helper on disconnect

 block/genhd.c         | 20 ++++++++++++++++++++
 drivers/block/loop.c  | 15 ++++-----------
 drivers/block/nbd.c   | 12 +++---------
 include/linux/genhd.h |  2 ++
 4 files changed, 29 insertions(+), 20 deletions(-)

-- 
2.11.0

To hide internal implementation and simplify some driver code,
this adds a helper to invalidate the gendisk. It will clean the
gendisk's associated buffer/page caches and reset its internal
states.

Signed-off-by: Xie Yongji <redacted>
---
 block/genhd.c         | 20 ++++++++++++++++++++
 include/linux/genhd.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index 7b6e5e1cf956..876c0e478baf 100644
--- a/block/genhd.c
+++ b/block/genhd.c

@@ -601,6 +601,26 @@ void del_gendisk(struct gendisk *disk)
 }
 EXPORT_SYMBOL(del_gendisk);
 
+/**
+ * invalidate_disk - invalidate the disk
+ * @disk: the struct gendisk to invalidate
+ *
+ * A helper to invalidates the disk. It will clean the disk's associated
+ * buffer/page caches and reset its internal states so that the disk
+ * can be reused by the drivers.
+ *
+ * Context: can sleep
+ */
+void invalidate_disk(struct gendisk *disk)
+{
+	struct block_device *bdev = disk->part0;
+
+	invalidate_bdev(bdev);
+	bdev->bd_inode->i_mapping->wb_err = 0;
+	set_capacity(disk, 0);
+}
+EXPORT_SYMBOL(invalidate_disk);
+
 /* sysfs access to bad-blocks list. */
 static ssize_t disk_badblocks_show(struct device *dev,
 					struct device_attribute *attr,

diff --git a/include/linux/genhd.h b/include/linux/genhd.h
index c68d83c87f83..ea815feb3626 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h

@@ -222,6 +222,8 @@ static inline int add_disk(struct gendisk *disk)
 }
 extern void del_gendisk(struct gendisk *gp);
 
+void invalidate_disk(struct gendisk *disk);
+
 void set_disk_ro(struct gendisk *disk, bool read_only);
 
 static inline int get_disk_ro(struct gendisk *disk)

-- 
2.11.0

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Use invalidate_disk() helper to simplify the code for gendisk
invalidation.

Signed-off-by: Xie Yongji <redacted>
Reviewed-by: Christoph Hellwig <hch@lst.de>
---
 drivers/block/loop.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 7bf4686af774..eab6906326cc 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c

@@ -1395,11 +1395,7 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 	blk_queue_logical_block_size(lo->lo_queue, 512);
 	blk_queue_physical_block_size(lo->lo_queue, 512);
 	blk_queue_io_min(lo->lo_queue, 512);
-	if (bdev) {
-		invalidate_bdev(bdev);
-		bdev->bd_inode->i_mapping->wb_err = 0;
-	}
-	set_capacity(lo->lo_disk, 0);
+	invalidate_disk(lo->lo_disk);
 	loop_sysfs_exit(lo);
 	if (bdev) {
 		/* let user-space know about this change */

-- 
2.11.0

The lo->lo_device can't be null if the lo->lo_backing_file is set.
So let's remove the unnecessary bdev checks and the entire bdev
variable in __loop_clr_fd() since the lo->lo_backing_file is already
checked before.

Signed-off-by: Xie Yongji <redacted>
---
 drivers/block/loop.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index eab6906326cc..980b538c008a 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c

@@ -1329,7 +1329,6 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 {
 	struct file *filp = NULL;
 	gfp_t gfp = lo->old_gfp_mask;
-	struct block_device *bdev = lo->lo_device;
 	int err = 0;
 	bool partscan = false;
 	int lo_number;

@@ -1397,16 +1396,14 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
 	blk_queue_io_min(lo->lo_queue, 512);
 	invalidate_disk(lo->lo_disk);
 	loop_sysfs_exit(lo);
-	if (bdev) {
-		/* let user-space know about this change */
-		kobject_uevent(&disk_to_dev(bdev->bd_disk)->kobj, KOBJ_CHANGE);
-	}
+	/* let user-space know about this change */
+	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
 	mapping_set_gfp_mask(filp->f_mapping, gfp);
 	/* This is safe: open() is still holding a reference. */
 	module_put(THIS_MODULE);
 	blk_mq_unfreeze_queue(lo->lo_queue);
 
-	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN && bdev;
+	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
 	lo_number = lo->lo_number;
 	disk_force_media_change(lo->lo_disk, DISK_EVENT_MEDIA_CHANGE);
 out_unlock:

-- 
2.11.0

On Wed, Sep 22, 2021 at 08:37:10PM +0800, Xie Yongji wrote:

The lo->lo_device can't be null if the lo->lo_backing_file is set.
So let's remove the unnecessary bdev checks and the entire bdev
variable in __loop_clr_fd() since the lo->lo_backing_file is already
checked before.

Signed-off-by: Xie Yongji <redacted>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

When a nbd device encounters a writeback error, that error will
get propagated to the bd_inode's wb_err field. Then if this nbd
device's backend is disconnected and another is attached, we will
get back the previous writeback error on fsync, which is unexpected.

To fix it, let's use invalidate_disk() helper to invalidate the
disk on disconnect instead of just setting disk's capacity to zero.

Signed-off-by: Xie Yongji <redacted>
---
 drivers/block/nbd.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 5170a630778d..19f2e1247416 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c

@@ -305,14 +305,6 @@ static void nbd_mark_nsock_dead(struct nbd_device *nbd, struct nbd_sock *nsock,
 	nsock->sent = 0;
 }
 
-static void nbd_size_clear(struct nbd_device *nbd)
-{
-	if (nbd->config->bytesize) {
-		set_capacity(nbd->disk, 0);
-		kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
-	}
-}
-
 static int nbd_set_size(struct nbd_device *nbd, loff_t bytesize,
 		loff_t blksize)
 {

@@ -1232,7 +1224,9 @@ static void nbd_config_put(struct nbd_device *nbd)
 					&nbd->config_lock)) {
 		struct nbd_config *config = nbd->config;
 		nbd_dev_dbg_close(nbd);
-		nbd_size_clear(nbd);
+		invalidate_disk(nbd->disk);
+		if (nbd->config->bytesize)
+			kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
 		if (test_and_clear_bit(NBD_RT_HAS_PID_FILE,
 				       &config->runtime_flags))
 			device_remove_file(disk_to_dev(nbd->disk), &pid_attr);

-- 
2.11.0

On Wed, Sep 22, 2021 at 08:37:11PM +0800, Xie Yongji wrote:

When a nbd device encounters a writeback error, that error will
get propagated to the bd_inode's wb_err field. Then if this nbd
device's backend is disconnected and another is attached, we will
get back the previous writeback error on fsync, which is unexpected.

To fix it, let's use invalidate_disk() helper to invalidate the
disk on disconnect instead of just setting disk's capacity to zero.

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Ping

On Wed, Sep 22, 2021 at 8:37 PM Xie Yongji [off-list ref] wrote:

This series comes from Christoph Hellwig's suggestion [1]. Some block
device drivers such as loop driver and nbd driver need to invalidate
the gendisk when the backend is detached so that the gendisk can be
reused by the new backend. Now the invalidation is done in device
driver with their own ways. To avoid code duplication and hide
some internals of the implementation, this series adds a block layer
helper and makes both loop driver and nbd driver use it.

[1] https://lore.kernel.org/all/YTmqJHd7YWAQ2lZ7@infradead.org/

V1 to V2:
- Rename invalidate_gendisk() to invalidate_disk()
- Add a cleanup patch to remove bdev checks and bdev variable in __loop_clr_fd()

Xie Yongji (4):
  block: Add invalidate_disk() helper to invalidate the gendisk
  loop: Use invalidate_disk() helper to invalidate gendisk
  loop: Remove the unnecessary bdev checks and unused bdev variable
  nbd: Use invalidate_disk() helper on disconnect

 block/genhd.c         | 20 ++++++++++++++++++++
 drivers/block/loop.c  | 15 ++++-----------
 drivers/block/nbd.c   | 12 +++---------
 include/linux/genhd.h |  2 ++
 4 files changed, 29 insertions(+), 20 deletions(-)

--
2.11.0

On Wed, 22 Sep 2021 20:37:07 +0800, Xie Yongji wrote:

This series comes from Christoph Hellwig's suggestion [1]. Some block
device drivers such as loop driver and nbd driver need to invalidate
the gendisk when the backend is detached so that the gendisk can be
reused by the new backend. Now the invalidation is done in device
driver with their own ways. To avoid code duplication and hide
some internals of the implementation, this series adds a block layer
helper and makes both loop driver and nbd driver use it.

[...]

Applied, thanks!

[1/4] block: Add invalidate_disk() helper to invalidate the gendisk
      commit: f059a1d2e23a165bf86e33673c6a7535a08c6341
[2/4] loop: Use invalidate_disk() helper to invalidate gendisk
      commit: e515be8f3b3e63be4c5e91dc6620483ed0990a0c
[3/4] loop: Remove the unnecessary bdev checks and unused bdev variable
      commit: 19f553db2ac03cb8407ec8efb8e140951afdfb87
[4/4] nbd: Use invalidate_disk() helper on disconnect
      commit: 435c2acb307f19acc791b4295e29cc53a82bd24d

Best regards,
-- 
Jens Axboe