From: Christoph Hellwig <hch@infradead.org>
To: Luis Chamberlain <mcgrof@kernel.org>
Cc: axboe@kernel.dk, hare@suse.de, bvanassche@acm.org,
ming.lei@redhat.com, hch@infradead.org, jack@suse.cz,
osandov@fb.com, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/6] block: move disk invalidation from del_gendisk() into a helper
Date: Thu, 15 Jul 2021 08:46:11 +0100 [thread overview]
Message-ID: <YO/nw2qDRg3yQDJj@infradead.org> (raw)
In-Reply-To: <20210715045531.420201-4-mcgrof@kernel.org>
I don't really like many of these helpers as they are rather
confusing. Below is a patch ontop of your whole series that
massages it into something I find much easier to follow.
The big caveats are:
- this moves the bdi link creation earlier, which is safe and probably
preferable. (should go into a prep patch).
- del_gendisk now needs to cope with not set up events and integrity
kobject
I also have a bunch of changes to del_gendisk which might require
backporting, let me try to get those out ASAP.
diff --git a/block/blk-integrity.c b/block/blk-integrity.c
index e46f47f2dec9..3135adab2e7c 100644
--- a/block/blk-integrity.c
+++ b/block/blk-integrity.c
@@ -446,6 +446,8 @@ int blk_integrity_add(struct gendisk *disk)
void blk_integrity_del(struct gendisk *disk)
{
+ if (!disk->integrity_kobj.state_initialized)
+ return;
kobject_uevent(&disk->integrity_kobj, KOBJ_REMOVE);
kobject_del(&disk->integrity_kobj);
kobject_put(&disk->integrity_kobj);
diff --git a/block/disk-events.c b/block/disk-events.c
index 0262784be34c..af7d7249eefa 100644
--- a/block/disk-events.c
+++ b/block/disk-events.c
@@ -458,7 +458,8 @@ void disk_del_events(struct gendisk *disk)
disk_block_events(disk);
mutex_lock(&disk_events_mutex);
- list_del_init(&disk->ev->node);
+ if (!list_empty(&disk->ev->node))
+ list_del_init(&disk->ev->node);
mutex_unlock(&disk_events_mutex);
}
}
diff --git a/block/genhd.c b/block/genhd.c
index c6c9c196ff27..0c8c71d78536 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -390,68 +390,8 @@ static void disk_scan_partitions(struct gendisk *disk)
blkdev_put(bdev, FMODE_READ);
}
-static void disk_announce(struct gendisk *disk)
-{
- struct device *ddev = disk_to_dev(disk);
-
- /* announce the disk and partitions after all partitions are created */
- dev_set_uevent_suppress(ddev, 0);
- disk_uevent(disk, KOBJ_ADD);
-}
-
-static void unregister_disk_partitions(struct gendisk *disk)
-{
- mutex_lock(&disk->open_mutex);
- disk->flags &= ~GENHD_FL_UP;
- blk_drop_partitions(disk);
- mutex_unlock(&disk->open_mutex);
-
- fsync_bdev(disk->part0);
- __invalidate_device(disk->part0, true);
-
- /*
- * Unhash the bdev inode for this device so that it can't be looked
- * up any more even if openers still hold references to it.
- */
- remove_inode_hash(disk->part0->bd_inode);
-
- set_capacity(disk, 0);
-}
-
-static void disk_invalidate(struct gendisk *disk)
-{
- if (!(disk->flags & GENHD_FL_HIDDEN)) {
- sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
-
- /*
- * Unregister bdi before releasing device numbers (as they can
- * get reused and we'd get clashes in sysfs).
- */
- bdi_unregister(disk->queue->backing_dev_info);
- }
-
- blk_unregister_queue(disk);
-
- kobject_put(disk->part0->bd_holder_dir);
- kobject_put(disk->slave_dir);
-
- part_stat_set_all(disk->part0, 0);
- disk->part0->bd_stamp = 0;
- if (!sysfs_deprecated)
- sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
- pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
- device_del(disk_to_dev(disk));
-}
-
-static void unregister_disk(struct gendisk *disk)
-{
- unregister_disk_partitions(disk);
- disk_invalidate(disk);
-}
-
-static int __must_check register_disk(struct device *parent,
- struct gendisk *disk,
- const struct attribute_group **groups)
+static int register_disk(struct device *parent, struct gendisk *disk,
+ const struct attribute_group **groups)
{
struct device *ddev = disk_to_dev(disk);
int err;
@@ -498,20 +438,22 @@ static int __must_check register_disk(struct device *parent,
if (disk->flags & GENHD_FL_HIDDEN)
return 0;
- disk_scan_partitions(disk);
- disk_announce(disk);
-
if (disk->queue->backing_dev_info->dev) {
err = sysfs_create_link(&ddev->kobj,
- &disk->queue->backing_dev_info->dev->kobj,
- "bdi");
+ &disk->queue->backing_dev_info->dev->kobj, "bdi");
if (WARN_ON(err))
goto exit_del_block_depr;
}
+
+ disk_scan_partitions(disk);
+
+ /* announce the disk and partitions after all partitions are created */
+ dev_set_uevent_suppress(ddev, 0);
+ disk_uevent(disk, KOBJ_ADD);
+
return 0;
exit_del_block_depr:
- unregister_disk_partitions(disk);
if (!sysfs_deprecated)
sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
exit_del_device:
@@ -614,6 +556,7 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
bdi_set_owner(bdi, dev);
bdev_add(disk->part0, dev->devt);
}
+
ret = register_disk(parent, disk, groups);
if (ret)
goto exit_unregister_bdi;
@@ -628,13 +571,11 @@ static int __device_add_disk(struct device *parent, struct gendisk *disk,
ret = blk_integrity_add(disk);
if (ret)
- goto exit_del_events;
+ goto exit_unregister_disk;
return 0;
-exit_del_events:
- disk_del_events(disk);
exit_unregister_disk:
- unregister_disk(disk);
+ del_gendisk(disk);
return ret;
exit_unregister_bdi:
@@ -691,7 +632,44 @@ void del_gendisk(struct gendisk *disk)
blk_integrity_del(disk);
disk_del_events(disk);
- unregister_disk(disk);
+
+ mutex_lock(&disk->open_mutex);
+ disk->flags &= ~GENHD_FL_UP;
+ blk_drop_partitions(disk);
+ mutex_unlock(&disk->open_mutex);
+
+ fsync_bdev(disk->part0);
+ __invalidate_device(disk->part0, true);
+
+ /*
+ * Unhash the bdev inode for this device so that it can't be looked
+ * up any more even if openers still hold references to it.
+ */
+ remove_inode_hash(disk->part0->bd_inode);
+
+ set_capacity(disk, 0);
+
+ if (!(disk->flags & GENHD_FL_HIDDEN)) {
+ sysfs_remove_link(&disk_to_dev(disk)->kobj, "bdi");
+
+ /*
+ * Unregister bdi before releasing device numbers (as they can
+ * get reused and we'd get clashes in sysfs).
+ */
+ bdi_unregister(disk->queue->backing_dev_info);
+ }
+
+ blk_unregister_queue(disk);
+
+ kobject_put(disk->part0->bd_holder_dir);
+ kobject_put(disk->slave_dir);
+
+ part_stat_set_all(disk->part0, 0);
+ disk->part0->bd_stamp = 0;
+ if (!sysfs_deprecated)
+ sysfs_remove_link(block_depr, dev_name(disk_to_dev(disk)));
+ pm_runtime_set_memalloc_noio(disk_to_dev(disk), false);
+ device_del(disk_to_dev(disk));
}
EXPORT_SYMBOL(del_gendisk);
next prev parent reply other threads:[~2021-07-15 7:47 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-07-15 4:55 [PATCH v2 0/6] block: add error handling for *add_disk*() Luis Chamberlain
2021-07-15 4:55 ` [PATCH v2 1/6] block: refcount the request_queue early in __device_add_disk() Luis Chamberlain
2021-07-15 7:02 ` Christoph Hellwig
2021-07-15 4:55 ` [PATCH v2 2/6] block: move disk announce work from register_disk() to a helper Luis Chamberlain
2021-07-15 7:05 ` Christoph Hellwig
2021-07-15 7:16 ` Christoph Hellwig
2021-07-15 4:55 ` [PATCH v2 3/6] block: move disk invalidation from del_gendisk() into " Luis Chamberlain
2021-07-15 7:46 ` Christoph Hellwig [this message]
2021-07-15 4:55 ` [PATCH v2 4/6] block: move disk unregistration work from del_gendisk() to " Luis Chamberlain
2021-07-15 4:55 ` [PATCH v2 5/6] block: add initial error handling for *add_disk()* and friends Luis Chamberlain
2021-07-15 7:57 ` Christoph Hellwig
2021-07-15 4:55 ` [PATCH v2 6/6] block: skip queue if NULL on blk_cleanup_queue() Luis Chamberlain
2021-07-15 7:11 ` Christoph Hellwig
2021-07-15 19:07 ` Luis Chamberlain
2021-07-19 9:50 ` Christoph Hellwig
2021-07-19 23:00 ` Luis Chamberlain
2021-07-15 7:23 ` [PATCH v2 0/6] block: add error handling for *add_disk*() Christoph Hellwig
2021-07-15 19:34 ` Luis Chamberlain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YO/nw2qDRg3yQDJj@infradead.org \
--to=hch@infradead.org \
--cc=axboe@kernel.dk \
--cc=bvanassche@acm.org \
--cc=hare@suse.de \
--cc=jack@suse.cz \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=ming.lei@redhat.com \
--cc=osandov@fb.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).