[libvirt] [PATCHv2 2/2] qemu: add new disk device='lun' for bus='virtio' & type='block'
Laine Stump
laine at laine.org
Thu Jan 5 15:59:43 UTC 2012
On 01/05/2012 03:44 AM, Paolo Bonzini wrote:
> Summary: two nits, one in the docs and one at the end of this email.
>
> [Osier, I'm CCing you because there is some food for thought for SCSI].
>
> On 01/05/2012 05:17 AM, Laine Stump wrote:
>> diff --git a/docs/formatdomain.html.in b/docs/formatdomain.html.in
>> index 18b7e22..dcdf91f 100644
>> --- a/docs/formatdomain.html.in
>> +++ b/docs/formatdomain.html.in
>> @@ -1001,8 +1001,16 @@
>> "block", "dir", or "network"
>> and refers to the underlying source for the disk. The optional
>> <code>device</code> attribute indicates how the disk is to be exposed
>> - to the guest OS. Possible values for this attribute are "floppy", "disk"
>> - and "cdrom", defaulting to "disk". The
>> + to the guest OS. Possible values for this attribute are
>> + "floppy", "disk", "cdrom", and "lun", defaulting to
>> + "disk". "lun" (<span class="since">since 0.9.10</span>) is only
>> + valid when type is "block" and the target element's "bus"
>> + attribute is "virtio", and behaves identically to "disk",
>> + except that generic SCSI commands from the guest are accepted
> What about "are forwarded to the disk"?
Okay, I'm adding that to the end of the sentence.
> This is also true in the SCSI
> case (for SCSI, "block" will emulate commands rather than fail them; but
> for "lun" the behavior is identical).
We can add that to the docs when virtio-blk-scsi support is added.
>> + - also note that device='lun' will only be recognized for
>> + actual raw devices, never for individual partitions or LVM
>> + partitions (in those cases, the kernel will reject the generic
>> + SCSI commands, making it identical to device='disk'). The
> Perhaps add a note that QEMU may fail to start? Again, this is not the
> case for virtio but it will be for "scsi".
I'd rather add that information when it actually is true (i.e. when
virtio-blk-scsi support is put in).
>> optional<code>snapshot</code> attribute indicates the default
>> behavior of the disk during disk snapshots: "internal"
>> requires a file format such as qcow2 that can store both the
>> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng
>> index 7a8f7f4..2f8bec5 100644
>> --- a/docs/schemas/domaincommon.rng
>> +++ b/docs/schemas/domaincommon.rng
>> @@ -794,6 +794,7 @@
>> <value>floppy</value>
>> <value>disk</value>
>> <value>cdrom</value>
>> +<value>lun</value>
>> </choice>
>> </attribute>
>> </optional>
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index 21113c6..a06942b 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -157,7 +157,8 @@ VIR_ENUM_IMPL(virDomainDisk, VIR_DOMAIN_DISK_TYPE_LAST,
>> VIR_ENUM_IMPL(virDomainDiskDevice, VIR_DOMAIN_DISK_DEVICE_LAST,
>> "disk",
>> "cdrom",
>> - "floppy")
>> + "floppy",
>> + "lun")
>>
>> VIR_ENUM_IMPL(virDomainDiskBus, VIR_DOMAIN_DISK_BUS_LAST,
>> "ide",
>> @@ -3093,7 +3094,8 @@ virDomainDiskDefParseXML(virCapsPtr caps,
>> if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM)
>> def->readonly = 1;
>>
>> - if (def->device == VIR_DOMAIN_DISK_DEVICE_DISK&&
>> + if ((def->device == VIR_DOMAIN_DISK_DEVICE_DISK ||
>> + def->device == VIR_DOMAIN_DISK_DEVICE_LUN)&&
>> !STRPREFIX((const char *)target, "hd")&&
>> !STRPREFIX((const char *)target, "sd")&&
>> !STRPREFIX((const char *)target, "vd")&&
> I wonder if you should extend this check to VIR_DOMAIN_DISK_DEVICE_CDROM
> as well.
Won't the list of names be different? ("sr")? At any rate, that's an
orthogonal issue, and should be addressed with a separate patch.
>> + if (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN) {
>> + /* make sure that both the bus and the qemu binary support
>> + * type='lun' (SG_IO).
>> + */
>> + if (disk->bus != VIR_DOMAIN_DISK_BUS_VIRTIO) {
>> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("disk device='lun' is not supported for bus='%s'"),
>> + bus);
>> + goto error;
>> + }
>> + if (disk->type != VIR_DOMAIN_DISK_TYPE_BLOCK) {
>> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED,
>> + _("disk device='lun' is not supported for type='%s'"),
>> + virDomainDiskTypeToString(disk->type));
>> + goto error;
>> + }
>> + if (!qemuCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SG_IO)) {
>> + qemuReportError(VIR_ERR_CONFIG_UNSUPPORTED, "%s",
>> + _("disk device='lun' is not supported by this QEMU"));
>> + goto error;
>> + }
>> + }
>> +
>> switch (disk->bus) {
>> case VIR_DOMAIN_DISK_BUS_IDE:
>> virBufferAddLit(&opt, "ide-drive");
>> @@ -2050,6 +2074,14 @@ qemuBuildDriveDevStr(virDomainDiskDefPtr disk,
>> virBufferAsprintf(&opt, ",event_idx=%s",
>> virDomainVirtioEventIdxTypeToString(disk->event_idx));
>> }
>> + if (qemuCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_BLK_SCSI)) {
>> + /* if sg_io is true but the scsi option isn't supported,
>> + * that means it's just always on in this version of qemu.
>> + */
>> + virBufferAsprintf(&opt, ",scsi=%s",
>> + (disk->device == VIR_DOMAIN_DISK_DEVICE_LUN)
>> + ? "on" : "off");
>> + }
>> if (qemuBuildDeviceAddressStr(&opt,&disk->info, qemuCaps)< 0)
>> goto error;
>> break;
>> @@ -4241,6 +4273,7 @@ qemuBuildCommandLine(virConnectPtr conn,
>> bootFloppy = 0;
>> break;
>> case VIR_DOMAIN_DISK_DEVICE_DISK:
>> + case VIR_DOMAIN_DISK_DEVICE_LUN:
>> bootindex = bootDisk;
>> bootDisk = 0;
>> break;
> Suboptimal, because a type='lun' disk could well be a CD. :/ But
> short of supporting bootindex directly in the<disk> and<network>
> items, we cannot do much about it though.
So this is a problem, but not fixable now, correct?
>> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
>> index 82bab67..3c55216 100644
>> --- a/src/qemu/qemu_driver.c
>> +++ b/src/qemu/qemu_driver.c
>> @@ -4987,6 +4987,7 @@ qemuDomainAttachDeviceDiskLive(virConnectPtr conn,
>> ret = qemuDomainChangeEjectableMedia(driver, vm, disk, false);
>> break;
> Side note: this fails if you're trying to hot-plug a SCSI CD drive.
> Probably this error condition in qemuDomainChangeEjectableMedia:
>
> for (i = 0 ; i< vm->def->ndisks ; i++) {
> if (vm->def->disks[i]->bus == disk->bus&&
> STREQ(vm->def->disks[i]->dst, disk->dst)) {
> origdisk = vm->def->disks[i];
> break;
> }
> }
>
> if (!origdisk) {
> qemuReportError(VIR_ERR_INTERNAL_ERROR,
> _("No device with bus '%s' and target '%s'"),
> virDomainDiskBusTypeToString(disk->bus),
> disk->dst);
> return -1;
> }
>
> should instead fall back to device hot-plug for VIR_DOMAIN_DISK_BUS_SCSI.
Just to verify - this is another thing that you've noticed in the code,
but is orthogonal to this patch, correct?
>> case VIR_DOMAIN_DISK_DEVICE_DISK:
>> + case VIR_DOMAIN_DISK_DEVICE_LUN:
>> if (disk->bus == VIR_DOMAIN_DISK_BUS_USB)
>> ret = qemuDomainAttachUsbMassstorageDevice(conn, driver, vm,
>> disk);
>> @@ -5109,6 +5110,7 @@ qemuDomainDetachDeviceDiskLive(struct qemud_driver *driver,
>>
>> switch (disk->device) {
>> case VIR_DOMAIN_DISK_DEVICE_DISK:
>> + case VIR_DOMAIN_DISK_DEVICE_LUN:
>> if (disk->bus == VIR_DOMAIN_DISK_BUS_VIRTIO)
>> ret = qemuDomainDetachPciDiskDevice(driver, vm, dev);
>> else if (disk->bus == VIR_DOMAIN_DISK_BUS_SCSI)
>> @@ -9607,7 +9609,8 @@ static int qemuDomainSnapshotIsAllowed(virDomainObjPtr vm)
>> * that succeed as well
>> */
>> for (i = 0; i< vm->def->ndisks; i++) {
>> - if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK&&
>> + if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK ||
>> + vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN)&&
>> (!vm->def->disks[i]->driverType ||
>> STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) {
>> qemuReportError(VIR_ERR_OPERATION_INVALID,
> I think that we should fail for device='lun' and format != 'raw'.
Truthfully, I didn't totally understand what that code was doing, but it
seemed appropriate to at least duplicate the behavior of DEVICE_DISK :-)
Looking at it some more, I *think* what is necessary, is to always
return false if there is a disk that is DEVICE_LUN, since really the
only thing that can be snapshotted is a qcow2 disk, and DEVICE_LUN disks
by definition are never qcow2. So does it seem reasonable to change this to:
- if (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK&&
- (!vm->def->disks[i]->driverType ||
- STRNEQ(vm->def->disks[i]->driverType, "qcow2"))) {
+ if ((vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_LUN) ||
+ (vm->def->disks[i]->device == VIR_DOMAIN_DISK_DEVICE_DISK&&
+ STRNEQ_NULLABLE(vm->def->disks[i]->driverType, "qcow2"))) {
qemuReportError(VIR_ERR_OPERATION_INVALID,
???
More information about the libvir-list
mailing list