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

[Asias He <asias@redhat.com>]

On 12/20/2012 07:27 PM, Alexander Graf wrote:
> 
> On 20.12.2012, at 11:54, Michael S. Tsirkin wrote:
> 
>> On Wed, Dec 19, 2012 at 08:40:15PM +0100, Alexander Graf wrote:
>>> 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 <agraf@suse.de>
>>> ---
>>> 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