[libvirt] [PATCH 6/6] Implement hotremove for SCSI disks

[Wolfgang Mauerer]

Daniel P. Berrange wrote:
> On Fri, Feb 26, 2010 at 02:09:19PM +0100, Wolfgang Mauerer wrote:
>> Recent qemu versions allow us to add disks dynamically into the system
>> via the drive_add/device_add mechanism. Removing them is now just a
>> matter of using device_del, and this patch adds the required bits
>> and pieces to libvirt.
> 
> There's one minor issue here, in that this removes the guest device,
> but does not remove the host side QEMU block driver state. We're
> still waiting for the 'drive_del' command to be implemented todo the
> latter bit.

I haven't checked with the qemu source code (Gerd is CC'ed), but the
drive associated with a device seems to disappear when the device is
removed - the drive ID can be reused with a different backing
file after removing the device, and I suppose that's sufficient for
libvirt (or did I miss a use case?):

(Add controller, drive and device)

(qemu) device_add lsi,id=controller
(qemu) drive_add 0 if=none,file=test.img,id=drive0
OK
(qemu) info block
ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
floppy0: type=floppy removable=1 locked=0 [not inserted]
sd0: type=floppy removable=1 locked=0 [not inserted]
drive0: type=hd removable=0 file=test.img ro=0 drv=raw encrypted=0
(qemu) device_add driver=scsi-disk,drive=drive0,bus=controller.0,id=device0
###########
(Remove the device -> the associated drive disappears)

(qemu) device_del device0
(qemu) info block
ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
floppy0: type=floppy removable=1 locked=0 [not inserted]
sd0: type=floppy removable=1 locked=0 [not inserted]
###########
(Add a new drive with the same ID as before, but different
backing file)

(qemu) drive_add 0 if=none,file=test2.img,id=drive0
OK
(qemu) device_add driver=scsi-disk,drive=drive0,bus=controller.0,id=device0
(qemu) info block
ide1-cd0: type=cdrom removable=1 locked=0 [not inserted]
floppy0: type=floppy removable=1 locked=0 [not inserted]
sd0: type=floppy removable=1 locked=0 [not inserted]
drive0: type=hd removable=0 file=test2.img ro=0 drv=raw encrypted=0

Gerd: Are you intending to add the drive_del feature, or is
the approach outlined above sufficient for drive hotplug/remove?
Respectively can there be any problems if we remove a device
associated with a disk and then re-create a drive/device pair
with the same IDs as before, but with a different configuration?

Best regards,

Wolfgang
> 	
>> Signed-off-by: Wolfgang Mauerer <wolfgang.mauerer at siemens.com>
>> Signed-off-by: Jan Kiszka <jan.kiszka at siemens.com>
>> ---
>>  src/qemu/qemu_driver.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 files changed, 57 insertions(+), 0 deletions(-)
>>
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index d683b1c..ecbb23d 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -5494,6 +5494,60 @@ qemuDomainFindOrCreateSCSIDiskController(struct qemud_driver *driver,
>>  }
>>  
>>  
>> +static int qemudDomainDetachSCSIDisk(struct qemud_driver *driver,
>> +                                     virDomainObjPtr vm,
>> +                                     virDomainDeviceDefPtr dev,
>> +                                     unsigned int qemuCmdFlags)
>> +{
>> +    int i;
>> +    qemuDomainObjPrivatePtr priv = vm->privateData;
>> +    virDomainDiskDefPtr detach = NULL;
>> +
>> +    if (!(qemuCmdFlags & QEMUD_CMD_FLAG_DEVICE)) {
>> +        return -1;
>> +    }
>> +
>> +    for (i = 0 ; i < vm->def->ndisks ; i++) {
>> +        if (STREQ(vm->def->disks[i]->dst, dev->data.disk->dst)) {
>> +            detach = vm->def->disks[i];
>> +            break;
>> +        }
>> +    }
>> +
>> +    if (!detach) {
>> +        qemuReportError(VIR_ERR_OPERATION_FAILED,
>> +                        _("disk %s not found"), dev->data.disk->dst);
>> +        return -1;
>> +    }
>> +
>> +    qemuDomainObjEnterMonitorWithDriver(driver, vm);
>> +    /* Note: drive_del does not exist, but device_del will
>> +       automatically erase the associated drive as well */
> 
> Are you sure about that ?  I was under the impression that 
> device_del would leave the drive dangling unused.
> 
>> +    if (qemuMonitorDelDevice(priv->mon, detach->info.alias)) {
>> +        qemuDomainObjExitMonitor(vm);
>> +        return -1;
>> +    }
>> +    qemuDomainObjExitMonitorWithDriver(driver, vm);
>> +
>> +    if (vm->def->ndisks > 1) {
>> +        memmove(vm->def->disks + i,
>> +                vm->def->disks + i + 1,
>> +                sizeof(*vm->def->disks) *
>> +                (vm->def->ndisks - (i + 1)));
>> +        vm->def->ndisks--;
>> +        if (VIR_REALLOC_N(vm->def->disks, vm->def->ndisks) < 0) {
>> +            /* ignore, harmless */
>> +        }
>> +    } else {
>> +        VIR_FREE(vm->def->disks);
>> +        vm->def->ndisks = 0;
>> +    }
>> +    virDomainDiskDefFree(detach);
>> +
>> +    return 0;
>> +}
>> +
>> +
>>  static int qemudDomainAttachSCSIDisk(struct qemud_driver *driver,
>>                                       virDomainObjPtr vm,
>>                                       virDomainDiskDefPtr disk,
>> @@ -6563,6 +6617,9 @@ static int qemudDomainDetachDevice(virDomainPtr dom,
>>          dev->data.disk->device == VIR_DOMAIN_DISK_DEVICE_DISK &&
>>          dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO) {
>>          ret = qemudDomainDetachPciDiskDevice(driver, vm, dev);
>> +    } else if(dev->type == VIR_DOMAIN_DEVICE_DISK &&
>> +              dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_SCSI) {
>> +        ret = qemudDomainDetachSCSIDisk(driver, vm, dev, qemuCmdFlags);
>>      } else if (dev->type == VIR_DOMAIN_DEVICE_NET) {
>>          ret = qemudDomainDetachNetDevice(driver, vm, dev);
>>      } else if (dev->type == VIR_DOMAIN_DEVICE_CONTROLLER) {
> 
> Regards,
> Daniel