Re: [PATCH] virtio-blk: Don't free ida when disk is in use

On 12/20/2012 07:27 PM, Alexander Graf wrote:

On 20.12.2012, at 11:54, Michael S. Tsirkin wrote:

quoted

On Wed, Dec 19, 2012 at 08:40:15PM +0100, Alexander Graf wrote:

quoted

When a file system is mounted on a virtio-blk disk, we then remove it
and then reattach it, the reattached disk gets the same disk name and
ids as the hot removed one.

This leads to very nasty effects - mostly rendering the newly attached
device completely unusable.

Trying what happens when I do the same thing with a USB device, I saw
that the sd node simply doesn't get free'd when a device gets forcefully
removed.

Imitate the same behavior for vd devices. This way broken vd devices
simply are never free'd and newly attached ones keep working just fine.

Signed-off-by: Alexander Graf <redacted>
---
drivers/block/virtio_blk.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 0bdde8f..07a18e2 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c

@@ -889,6 +889,7 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)

{
	struct virtio_blk *vblk = vdev->priv;
	int index = vblk->index;
+	int refc;

	/* Prevent config work handler from accessing the device. */
	mutex_lock(&vblk->config_lock);

@@ -903,11 +904,15 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)

	flush_work(&vblk->config_work);

+	refc = atomic_read(&disk_to_dev(vblk->disk)->kobj.kref.refcount);
	put_disk(vblk->disk);
	mempool_destroy(vblk->pool);
	vdev->config->del_vqs(vdev);
	kfree(vblk);
-	ida_simple_remove(&vd_index_ida, index);
+
+	/* Only free device id if we don't have any users */
+	if (refc == 1)
+		ida_simple_remove(&vd_index_ida, index);
}

#ifdef CONFIG_PM

Network devices take the approach of retrying every second.
Donnu if it makes sense here.

I would rather think the 100% right approach would be a recursive unrolling of all users bottom to top. Force unmount. Force close all fd's. I'm not sure why that doesn't happen today, but it doesn't :).

Yes ;-)

Alex

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

-- 
Asias