Skip to contentSkip to conversation

: Fix crash in virtio_blk during modprobe ; rmmod ; modprobe]

PATCH In review Chris Lalancette 3 participants 11 messages May 21, 2008 – May 23, 2008

Subsystems (4)
KVM Block layer virtio core virtio block and SCSI drivers
lore
Share & tools
lore

Chris Lalancette

author
#0 lore raw

PATCH 1/1 : Fix crash in virtio_blk during modprobe ; rmmod ; modprobe]

(sent to the kvm mailing list erroneously first)

Fix a modprobe virtio_blk ; rmmod virtio_blk ; modprobe virtio_blk crash; this was basically because we weren't doing "del_gendisk()" in the remove path.

Signed-off-by Chris Lalancette <[email protected]>

commit 9ae82ccb26be0155ad81b2630090e85639a0dc56

Author: Chris Lalancette <[email protected]>
Date:   Fri May 16 15:31:06 2008 -0400

Fix a modprobe virtio_blk ; rmmod virtio_blk ; modprobe virtio_blk crash; this was basically because we weren't doing "del_gendisk()" in the remove path.

drivers/block/virtio_blk.c +1 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
 	vdev->config->reset(vdev);
 
 	blk_cleanup_queue(vblk->disk->queue);
+	del_gendisk(vblk->disk);
 	put_disk(vblk->disk);
 	unregister_blkdev(major, "virtblk");
 	mempool_destroy(vblk->pool);

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

#1 lore raw
Quoted 6 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +0 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
         vdev->config->reset(vdev);
Quoted 6 lines


        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Jens CC'd: he's gentle with my dumb questions...
Rusty.
_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linux-foundation.org/mailman/listinfo/virtualization

#2 lore raw
In reply to Chris Lalancette
Quoted 6 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +0 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
         vdev->config->reset(vdev);
Quoted 6 lines


        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Jens CC'd: he's gentle with my dumb questions...
Rusty.

#3 lore raw
Quoted 18 lines · Rusty Russell
Quoted 6 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +0 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
         vdev->config->reset(vdev);
Quoted 6 lines


        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Quoted 1 line

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Signature · 1 line
Jens Axboe
#4 lore raw
In reply to Rusty Russell
Quoted 18 lines · Rusty Russell
Quoted 6 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +0 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
         vdev->config->reset(vdev);
Quoted 6 lines


        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Quoted 1 line

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Signature · 1 line
Jens Axboe
author
#5 lore raw
Quoted 18 lines · Jens Axboe
Quoted 17 lines · Rusty Russell

drivers/block/virtio_blk.c static void virtblk_remove(struct virtio_device *vdev) · line 300

Quoted 12 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +1 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
        vdev->config->reset(vdev);
 
        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Quoted 5 lines
Quoted 1 line

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Ah, good to know. Out of curiousity, why would del_gendisk() generate additional I/O?

Chris Lalancette

author
#6 lore raw
In reply to Jens Axboe
Quoted 18 lines · Jens Axboe
Quoted 17 lines · Rusty Russell

drivers/block/virtio_blk.c static void virtblk_remove(struct virtio_device *vdev) · line 300

Quoted 12 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +1 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
        vdev->config->reset(vdev);
 
        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Quoted 5 lines
Quoted 1 line

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Ah, good to know. Out of curiousity, why would del_gendisk() generate additional I/O?

Chris Lalancette

#7 lore raw
Quoted 19 lines · Chris Lalancette
Quoted 18 lines · Jens Axboe
Quoted 17 lines · Rusty Russell

drivers/block/virtio_blk.c static void virtblk_remove(struct virtio_device *vdev) · line 300

Quoted 12 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +1 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
        vdev->config->reset(vdev);
 
        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Ah, good to know. Out of curiousity, why would del_gendisk() generate additional I/O?

It does invalidate+sync.

Signature · 1 line
Jens Axboe
#8 lore raw
In reply to Chris Lalancette
Quoted 19 lines · Chris Lalancette
Quoted 18 lines · Jens Axboe
Quoted 17 lines · Rusty Russell

drivers/block/virtio_blk.c static void virtblk_remove(struct virtio_device *vdev) · line 300

Quoted 12 lines · On Wednesday 21 May 2008 23:12:39 Chris Lalancette wrote:
drivers/block/virtio_blk.c +1 -0
index 4962e62..c678ac5 100644
@@ -294,6 +294,7 @@ static void virtblk_remove(struct virtio_device *vdev)
        vdev->config->reset(vdev);
 
        blk_cleanup_queue(vblk->disk->queue);
+       del_gendisk(vblk->disk);
        put_disk(vblk->disk);
        unregister_blkdev(major, "virtblk");
        mempool_destroy(vblk->pool);

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Quoted 8 lines
Quoted 5 lines
Quoted 1 line

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Ah, good to know. Out of curiousity, why would del_gendisk() generate additional I/O?

It does invalidate+sync.

Signature · 1 line
Jens Axboe
#9 lore raw
In reply to Jens Axboe
Quoted 9 lines · On Thursday 22 May 2008 21:35:19 Jens Axboe wrote:
Quoted 5 lines · Rusty Russell

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Thanks Jens. Chris, I've fixed that up here.

Cheers,
Rusty.

#10 lore raw
In reply to Jens Axboe
Quoted 9 lines · On Thursday 22 May 2008 21:35:19 Jens Axboe wrote:
Quoted 5 lines · Rusty Russell

Thanks Chris, it seems reasonable and I'm sure it works (kinda hard to test here, it's my root block dev). Other drivers seem to do blk_cleanup_queue() *after* del_gendisk: does it matter?

Jens CC'd: he's gentle with my dumb questions... Rusty.

del_gendisk() can generate IO, so it would seem safer to do that _before_ putting the queue reference :-)

Thanks Jens. Chris, I've fixed that up here.

Cheers,
Rusty.