[PATCH 0/3] Add invalidate_gendisk() helper for drivers to invalidate the gendisk

[PATCH 0/3] Add invalidate_gendisk() helper for drivers to invalidate the gendisk

[Xie Yongji]

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/

Xie Yongji (3):
  block: Add invalidate_gendisk() helper to invalidate the gendisk
  loop: Use invalidate_gendisk() helper to invalidate gendisk
  nbd: Use invalidate_gendisk() helper on disconnect

 block/genhd.c         | 21 +++++++++++++++++++++
 drivers/block/loop.c  |  6 +-----
 drivers/block/nbd.c   | 12 +++---------
 include/linux/genhd.h |  1 +
 4 files changed, 26 insertions(+), 14 deletions(-)

-- 
2.11.0

[PATCH 1/3] block: Add invalidate_gendisk() helper to invalidate the gendisk

[Xie Yongji]

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 <xieyongji@bytedance.com>
---
 block/genhd.c         | 21 +++++++++++++++++++++
 include/linux/genhd.h |  1 +
 2 files changed, 22 insertions(+)

diff --git a/block/genhd.c b/block/genhd.c
index 7b6e5e1cf956..7d97fb93069a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -601,6 +601,27 @@ void del_gendisk(struct gendisk *disk)
 }
 EXPORT_SYMBOL(del_gendisk);
 
+/**
+ * invalidate_gendisk - invalidate the gendisk
+ * @disk: the struct gendisk to invalidate
+ *
+ * A helper to invalidates the gendisk. It will clean the gendisk's associated
+ * buffer/page caches and reset its internal states so that the gendisk
+ * can be reused by the drivers.
+ *
+ * Context: can sleep
+ */
+
+void invalidate_gendisk(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_gendisk);
+
 /* 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..a3b2f6b1b7e8 100644
--- a/include/linux/genhd.h
+++ b/include/linux/genhd.h
@@ -221,6 +221,7 @@ static inline int add_disk(struct gendisk *disk)
 	return device_add_disk(NULL, disk, NULL);
 }
 extern void del_gendisk(struct gendisk *gp);
+extern void invalidate_gendisk(struct gendisk *gp);
 
 void set_disk_ro(struct gendisk *disk, bool read_only);
 
-- 
2.11.0

Re: [PATCH 1/3] block: Add invalidate_gendisk() helper to invalidate the gendisk

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 07:25:55PM +0800, Xie Yongji wrote:
>  
> +/**
> + * invalidate_gendisk - invalidate the gendisk
> + * @disk: the struct gendisk to invalidate
> + *
> + * A helper to invalidates the gendisk. It will clean the gendisk's associated
> + * buffer/page caches and reset its internal states so that the gendisk
> + * can be reused by the drivers.
> + *
> + * Context: can sleep
> + */
> +
> +void invalidate_gendisk(struct gendisk *disk)

No need for the empty line.  Also I wonder if invalidate_disk might be a
better name - except for del_gendisk we don't really use _gendisk in
function names at all.

> +extern void invalidate_gendisk(struct gendisk *gp);

No need for the extern.  Also I'd name the argument disk, just as in
the actual implementation.

The actual functionality looks good to me despite these nitpicks.

Re: [PATCH 1/3] block: Add invalidate_gendisk() helper to invalidate the gendisk

[Yongji Xie]

On Mon, Sep 13, 2021 at 8:32 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Sep 13, 2021 at 07:25:55PM +0800, Xie Yongji wrote:
> >
> > +/**
> > + * invalidate_gendisk - invalidate the gendisk
> > + * @disk: the struct gendisk to invalidate
> > + *
> > + * A helper to invalidates the gendisk. It will clean the gendisk's associated
> > + * buffer/page caches and reset its internal states so that the gendisk
> > + * can be reused by the drivers.
> > + *
> > + * Context: can sleep
> > + */
> > +
> > +void invalidate_gendisk(struct gendisk *disk)
>
> No need for the empty line.  Also I wonder if invalidate_disk might be a
> better name - except for del_gendisk we don't really use _gendisk in
> function names at all.
>

Looks good to me.

> > +extern void invalidate_gendisk(struct gendisk *gp);
>
> No need for the extern.  Also I'd name the argument disk, just as in
> the actual implementation.
>

OK.

Thanks,
Yongji

[PATCH 2/3] loop: Use invalidate_gendisk() helper to invalidate gendisk

[Xie Yongji]

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

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 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..ddcf0428cdf9 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_gendisk(lo->lo_disk);
 	loop_sysfs_exit(lo);
 	if (bdev) {
 		/* let user-space know about this change */
-- 
2.11.0

Re: [PATCH 2/3] loop: Use invalidate_gendisk() helper to invalidate gendisk

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 07:25:56PM +0800, Xie Yongji wrote:
> Use invalidate_gendisk() helper to simplify the code for gendisk
> invalidation.

bdev can't be null here so this looks good:

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

Can you also add a cleanup patch to remove the bdev checks and the
entire bdev variable in __loop_clr_fd?

Re: [PATCH 2/3] loop: Use invalidate_gendisk() helper to invalidate gendisk

[Yongji Xie]

On Mon, Sep 13, 2021 at 8:35 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Sep 13, 2021 at 07:25:56PM +0800, Xie Yongji wrote:
> > Use invalidate_gendisk() helper to simplify the code for gendisk
> > invalidation.
>
> bdev can't be null here so this looks good:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
>
> Can you also add a cleanup patch to remove the bdev checks and the
> entire bdev variable in __loop_clr_fd?

Sure.

Thanks,
Yongji

[PATCH 3/3] nbd: Use invalidate_gendisk() helper on disconnect

[Xie Yongji]

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_gendisk() helper to invalidate the
disk on disconnect instead of just setting disk's capacity to zero.

Reported-by: Yi Xingchen <yixingchen@bytedance.com>
Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 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..64ae087d8767 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_gendisk(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

Re: [PATCH 3/3] nbd: Use invalidate_gendisk() helper on disconnect

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 07:25:57PM +0800, Xie Yongji wrote:
> +		invalidate_gendisk(nbd->disk);
> +		if (nbd->config->bytesize)
> +			kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);

I wonder if the invalidate helper should just use
set_capacity_and_notify to take care of the notification in the proper
way.  This adds an uevent to loop, and adds the RESIZE=1 argument to
nbd, but it feels like the right thing to do.

Re: [PATCH 3/3] nbd: Use invalidate_gendisk() helper on disconnect

[Yongji Xie]

On Mon, Sep 13, 2021 at 8:43 PM Christoph Hellwig <hch@infradead.org> wrote:
>
> On Mon, Sep 13, 2021 at 07:25:57PM +0800, Xie Yongji wrote:
> > +             invalidate_gendisk(nbd->disk);
> > +             if (nbd->config->bytesize)
> > +                     kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
>
> I wonder if the invalidate helper should just use
> set_capacity_and_notify to take care of the notification in the proper
> way.  This adds an uevent to loop, and adds the RESIZE=1 argument to
> nbd, but it feels like the right thing to do.

Looks like set_capacity_and_notify() would not do notification if we
set capacity to zero. How about calling kobject_uevent() directly in
the helper?

Thanks,
Yongji

Re: [PATCH 3/3] nbd: Use invalidate_gendisk() helper on disconnect

[Christoph Hellwig]

On Mon, Sep 13, 2021 at 09:04:12PM +0800, Yongji Xie wrote:
> On Mon, Sep 13, 2021 at 8:43 PM Christoph Hellwig <hch@infradead.org> wrote:
> >
> > On Mon, Sep 13, 2021 at 07:25:57PM +0800, Xie Yongji wrote:
> > > +             invalidate_gendisk(nbd->disk);
> > > +             if (nbd->config->bytesize)
> > > +                     kobject_uevent(&nbd_to_dev(nbd)->kobj, KOBJ_CHANGE);
> >
> > I wonder if the invalidate helper should just use
> > set_capacity_and_notify to take care of the notification in the proper
> > way.  This adds an uevent to loop, and adds the RESIZE=1 argument to
> > nbd, but it feels like the right thing to do.
> 
> Looks like set_capacity_and_notify() would not do notification if we
> set capacity to zero.

True.

> How about calling kobject_uevent() directly in
> the helper?

That's probably and improvement over letting the driver do it, so let's
go with that for now instead of adding ever more work to your plate.