On 20.12.2012, at 12:38, Michael S. Tsirkin wrote:

> On Thu, Dec 20, 2012 at 12:27:53PM +0100, 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 <ag...@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 :).
>> 
>> 
>> Alex
> 
> Especially force close fd's won't be easy. A simpler alternative
> would be preventing unplug by taking some reference count.

That's what happens when you do

<add>
mount /mnt
ls /mnt
<remove>

-- here remove failed for me --

But when you do

<add>
mount /mnt
<remove>

then remove works and we get the above problem.

I have to admit that I'm really not a big fan of preventing unplug though. In 
some cases, preventing simply doesn't work, for example when you unplug a USB 
stick or when your iscsi remote just vanishes.


Alex

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

Reply via email to