Re: Bisected, with rfc/patch - was Re: BUG: unable to handle kernel NULL pointer dereference at sysfs_do_create_link_sd (after mdadm)

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3201 bytes --]

On Thu, 23 Apr 2015 18:10:51 +0200 Christoph Hellwig <hch@lst.de> wrote:

> On Thu, Apr 23, 2015 at 06:03:14PM +1000, NeilBrown wrote:
> > On Thu, 23 Apr 2015 09:37:24 +0200 Christoph Hellwig <hch@lst.de> wrote:
> > 
> > > Plase fix your device name lifetimes.
> > 
> > Any chance you could be more explicit?
> >
> > The commit you identified doesn't seem to help much - md and dm are quite
> > different in this area.
> > 
> > It seems that it is no longer safe to call 'add_disk' between calling
> > 'del_gendisk' and bdi_destroy being called.  How can I find out if I am in
> > that window, or wait for bdi_destroy to be called?
> 
> The bdi is only around if the device is open, either through a device
> node, or through a blkdev_get from a file system.  If you get duplicate
> names that means you're trying to allocate a new gendisk while the old
> one is still around.
> 
> In theory you're fine once the device gets ->release called.

In practice .... the put_disk() shortly after the ->release call in
__blkdev_put() is what ultimately releases the name of the bdi - at least in
the cases where I get a crash.

> 
> Except that we can hold sysfs reference to the qeue, eww.  So for now
> try to follow the dm model, but I'll need to add a callback to the
> queue called once the request_queue actually is released for this.

I'm pretty sure that the md code is already as close to the "dm model" as it
meaningfully can be.

If I move bdi_destroy out of blk_release_queue (which really think is too
later) and place it in blk_cleanup_queue (which seems a credible place for
it), and then move the blk_cleanup_queue call in md_free up before the
del_gendisk() call (which is probably the right thing to do anyway, though dm
has the same order that md currently has) then I don't get any crashes and
I'm almost convince it is correct...

Thoughts?

Thanks,
NeilBrown


diff --git a/block/blk-core.c b/block/blk-core.c
index 794c3e7f01cf..66406474f0c4 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -552,6 +552,8 @@ void blk_cleanup_queue(struct request_queue *q)
 		q->queue_lock = &q->__queue_lock;
 	spin_unlock_irq(lock);
 
+	bdi_destroy(&q->backing_dev_info);
+
 	/* @q is and will stay empty, shutdown and put */
 	blk_put_queue(q);
 }
diff --git a/block/blk-sysfs.c b/block/blk-sysfs.c
index faaf36ade7eb..2b8fd302f677 100644
--- a/block/blk-sysfs.c
+++ b/block/blk-sysfs.c
@@ -522,8 +522,6 @@ static void blk_release_queue(struct kobject *kobj)
 
 	blk_trace_shutdown(q);
 
-	bdi_destroy(&q->backing_dev_info);
-
 	ida_simple_remove(&blk_queue_ida, q->id);
 	call_rcu(&q->rcu_head, blk_free_queue_rcu);
 }
diff --git a/drivers/md/md.c b/drivers/md/md.c
index d4f31e195e26..593a02476c78 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -4818,12 +4818,12 @@ static void md_free(struct kobject *ko)
 	if (mddev->sysfs_state)
 		sysfs_put(mddev->sysfs_state);
 
+	if (mddev->queue)
+		blk_cleanup_queue(mddev->queue);
 	if (mddev->gendisk) {
 		del_gendisk(mddev->gendisk);
 		put_disk(mddev->gendisk);
 	}
-	if (mddev->queue)
-		blk_cleanup_queue(mddev->queue);
 
 	kfree(mddev);
 }

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]