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
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
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
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
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.
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?
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.
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
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
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
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.