Re: [PATCH 2/8] md: implement ->free_disk
From: Logan Gunthorpe <logang@deltatee.com>
Date: 2022-07-12 23:14:57
Also in:
linux-block
Subsystem:
software raid (multiple disks) support, the rest · Maintainers:
Song Liu, Yu Kuai, Linus Torvalds
On 2022-07-12 01:03, Christoph Hellwig wrote:
quoted hunk ↗ jump to hunk
Ensure that all private data is only freed once all accesses are done. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/md/md.c | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-)diff --git a/drivers/md/md.c b/drivers/md/md.c index 861d6a9481b2e..ae076a7a87796 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c@@ -5581,11 +5581,6 @@ static void md_free(struct kobject *ko) del_gendisk(mddev->gendisk); put_disk(mddev->gendisk); } - percpu_ref_exit(&mddev->writes_pending); - - bioset_exit(&mddev->bio_set); - bioset_exit(&mddev->sync_set); - kfree(mddev); } static const struct sysfs_ops md_sysfs_ops = {@@ -7844,6 +7839,17 @@ static unsigned int md_check_events(struct gendisk *disk, unsigned int clearing) return ret; } +static void md_free_disk(struct gendisk *disk) +{ + struct mddev *mddev = disk->private_data; + + percpu_ref_exit(&mddev->writes_pending); + bioset_exit(&mddev->bio_set); + bioset_exit(&mddev->sync_set); + + kfree(mddev); +}
I still don't think this is entirely correct. There are error paths that will put the kobject before the disk is created and if they get hit then the kfree(mddev) will never be called and the memory will be leaked. Instead of creating an ugly special path for that, I came up with a solution that I think makes a bit more sense: the kobject is still freed in it's own free function, but the disk holds a reference to the kobject and drops it in its free function. The sysfs puts and del_gendisk are then moved into mddev_delayed_delete() so they happen earlier. Thoughts? Logan --
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 78e2588ed43e..330db78a5b38 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c@@ -5585,15 +5585,6 @@ static void md_free(struct kobject *ko) { struct mddev *mddev = container_of(ko, struct mddev, kobj); - if (mddev->sysfs_state) - sysfs_put(mddev->sysfs_state); - if (mddev->sysfs_level) - sysfs_put(mddev->sysfs_level); - - if (mddev->gendisk) { - del_gendisk(mddev->gendisk); - blk_cleanup_disk(mddev->gendisk); - } percpu_ref_exit(&mddev->writes_pending); bioset_exit(&mddev->bio_set);
@@ -5618,6 +5609,17 @@ static void mddev_delayed_delete(struct work_struct *ws) struct mddev *mddev = container_of(ws, struct mddev, del_work); kobject_del(&mddev->kobj); + + if (mddev->sysfs_state) + sysfs_put(mddev->sysfs_state); + if (mddev->sysfs_level) + sysfs_put(mddev->sysfs_level); + + if (mddev->gendisk) { + del_gendisk(mddev->gendisk); + blk_cleanup_disk(mddev->gendisk); + } + kobject_put(&mddev->kobj); }
@@ -5708,6 +5710,7 @@ int md_alloc(dev_t dev, char *name) else sprintf(disk->disk_name, "md%d", unit); disk->fops = &md_fops; + kobject_get(&mddev->kobj); disk->private_data = mddev; mddev->queue = disk->queue;
@@ -7858,6 +7861,13 @@ static unsigned int md_check_events(struct gendisk *disk, unsign> return ret; } +static void md_free_disk(struct gendisk *disk) +{ + struct mddev *mddev = disk->private_data; + + kobject_put(&mddev->kobj); +} + const struct block_device_operations md_fops = { .owner = THIS_MODULE,
@@ -7871,6 +7881,7 @@ const struct block_device_operations md_fops = .getgeo = md_getgeo, .check_events = md_check_events, .set_read_only = md_set_read_only, + .free_disk = md_free_disk, };