8:45 a.m.
It's easy to reproduce with two domains using the same SCSI
generic device, both with <shareable> specified. It's expected
to work. But the fact is:
% virsh start f20-1
error: Failed to start domain b
error: Requested operation is not valid: SCSI device 1:0:0:0 is in use by domain f20-0
Osier Yang (2):
util: Add "shareable" field for virSCSIDevice struct
qemu: Don't fail if the SCSI host device is shareable between domains
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 3 ++-
src/qemu/qemu_hostdev.c | 42 +++++++++++++++++++++++-----------------
src/security/security_apparmor.c | 3 ++-
src/security/security_dac.c | 6 ++++--
src/security/security_selinux.c | 6 ++++--
src/util/virscsi.c | 11 ++++++++++-
src/util/virscsi.h | 4 +++-
8 files changed, 50 insertions(+), 26 deletions(-)
--
1.8.1.4
8:45 a.m.
New subject: [libvirt] [PATCH 1/2] util: Add "shareable" field for virSCSIDevice struct
Unlike the host devices of other types, SCSI host device XML supports
"shareable" tag. This patch introduces it for the virSCSIDevice struct
for a later patch use (to detect if the SCSI device is shareable when
preparing the SCSI host device in QEMU driver).
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 3 ++-
src/qemu/qemu_hostdev.c | 9 ++++++---
src/security/security_apparmor.c | 3 ++-
src/security/security_dac.c | 6 ++++--
src/security/security_selinux.c | 6 ++++--
src/util/virscsi.c | 11 ++++++++++-
src/util/virscsi.h | 4 +++-
8 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2dbb8f8..68ca5da 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1671,6 +1671,7 @@ virSCSIDeviceGetDevName;
virSCSIDeviceGetName;
virSCSIDeviceGetReadonly;
virSCSIDeviceGetSgName;
+virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
virSCSIDeviceGetUsedBy;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a18955e..10b1131 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -295,7 +295,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly)) == NULL)
+ dev->readonly,
+ dev->shareable)) == NULL)
goto cleanup;
if (virSCSIDeviceFileIterate(scsi,
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dee61e7..86a463a 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -267,7 +267,8 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
virSCSIDeviceSetUsedBy(scsi, def->name);
@@ -1097,7 +1098,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) {
@@ -1395,7 +1397,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly))) {
+ hostdev->readonly,
+ hostdev->shareable))) {
VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain
%s",
hostdev->source.subsys.u.scsi.adapter,
hostdev->source.subsys.u.scsi.bus,
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index a9f04d2..86a033f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -833,7 +833,8 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cb7d322..0952df9 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -536,7 +536,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -653,7 +654,8 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 11c0c3b..37f755c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1353,7 +1353,8 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr
def,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -1544,7 +1545,8 @@
virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 7aca9e6..7462f9d 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -58,6 +58,7 @@ struct _virSCSIDevice {
const char *used_by; /* name of the domain using this dev */
bool readonly;
+ bool shareable;
};
struct _virSCSIDeviceList {
@@ -185,7 +186,8 @@ virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly)
+ bool readonly,
+ bool shareable)
{
virSCSIDevicePtr dev, ret = NULL;
char *sg = NULL;
@@ -201,6 +203,7 @@ virSCSIDeviceNew(const char *adapter,
dev->target = target;
dev->unit = unit;
dev->readonly = readonly;
+ dev->shareable= shareable;
if (!(sg = virSCSIDeviceGetSgName(adapter, bus, target, unit)))
goto cleanup;
@@ -311,6 +314,12 @@ virSCSIDeviceGetReadonly(virSCSIDevicePtr dev)
return dev->readonly;
}
+bool
+virSCSIDeviceGetShareable(virSCSIDevicePtr dev)
+{
+ return dev->shareable;
+}
+
int
virSCSIDeviceFileIterate(virSCSIDevicePtr dev,
virSCSIDeviceFileActor actor,
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index cce5df4..84890be 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -46,7 +46,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly);
+ bool readonly,
+ bool shareable);
void virSCSIDeviceFree(virSCSIDevicePtr dev);
void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
@@ -57,6 +58,7 @@ unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev);
bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev);
+bool virSCSIDeviceGetShareable(virSCSIDevicePtr dev);
/*
* Callback that will be invoked once for each file
--
1.8.1.4
9:53 a.m.
New subject: [libvirt] [PATCH 1/2] util: Add "shareable" field for virSCSIDevice struct
On 01/02/2014 09:45 AM, Osier Yang wrote:
Unlike the host devices of other types, SCSI host device XML
supports
"shareable" tag. This patch introduces it for the virSCSIDevice struct
for a later patch use (to detect if the SCSI device is shareable when
preparing the SCSI host device in QEMU driver).
---
src/libvirt_private.syms | 1 +
src/qemu/qemu_cgroup.c | 3 ++-
src/qemu/qemu_hostdev.c | 9 ++++++---
src/security/security_apparmor.c | 3 ++-
src/security/security_dac.c | 6 ++++--
src/security/security_selinux.c | 6 ++++--
src/util/virscsi.c | 11 ++++++++++-
src/util/virscsi.h | 4 +++-
8 files changed, 32 insertions(+), 11 deletions(-)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index 2dbb8f8..68ca5da 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -1671,6 +1671,7 @@ virSCSIDeviceGetDevName;
virSCSIDeviceGetName;
virSCSIDeviceGetReadonly;
virSCSIDeviceGetSgName;
+virSCSIDeviceGetShareable;
virSCSIDeviceGetTarget;
virSCSIDeviceGetUnit;
virSCSIDeviceGetUsedBy;
diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
index a18955e..10b1131 100644
--- a/src/qemu/qemu_cgroup.c
+++ b/src/qemu/qemu_cgroup.c
@@ -295,7 +295,8 @@ qemuSetupHostdevCGroup(virDomainObjPtr vm,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly)) == NULL)
+ dev->readonly,
+ dev->shareable)) == NULL)
goto cleanup;
if (virSCSIDeviceFileIterate(scsi,
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index dee61e7..86a463a 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -267,7 +267,8 @@ qemuUpdateActiveScsiHostdevs(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
virSCSIDeviceSetUsedBy(scsi, def->name);
@@ -1097,7 +1098,8 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly)))
+ hostdev->readonly,
+ hostdev->shareable)))
goto cleanup;
if (scsi && virSCSIDeviceListAdd(list, scsi) < 0) {
@@ -1395,7 +1397,8 @@ qemuDomainReAttachHostScsiDevices(virQEMUDriverPtr driver,
hostdev->source.subsys.u.scsi.bus,
hostdev->source.subsys.u.scsi.target,
hostdev->source.subsys.u.scsi.unit,
- hostdev->readonly))) {
+ hostdev->readonly,
+ hostdev->shareable))) {
VIR_WARN("Unable to reattach SCSI device %s:%d:%d:%d on domain
%s",
hostdev->source.subsys.u.scsi.adapter,
hostdev->source.subsys.u.scsi.bus,
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
index a9f04d2..86a033f 100644
--- a/src/security/security_apparmor.c
+++ b/src/security/security_apparmor.c
@@ -833,7 +833,8 @@ AppArmorSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_dac.c b/src/security/security_dac.c
index cb7d322..0952df9 100644
--- a/src/security/security_dac.c
+++ b/src/security/security_dac.c
@@ -536,7 +536,8 @@ virSecurityDACSetSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -653,7 +654,8 @@ virSecurityDACRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c
index 11c0c3b..37f755c 100644
--- a/src/security/security_selinux.c
+++ b/src/security/security_selinux.c
@@ -1353,7 +1353,8 @@ virSecuritySELinuxSetSecurityHostdevSubsysLabel(virDomainDefPtr
def,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
@@ -1544,7 +1545,8 @@
virSecuritySELinuxRestoreSecurityHostdevSubsysLabel(virSecurityManagerPtr mgr,
dev->source.subsys.u.scsi.bus,
dev->source.subsys.u.scsi.target,
dev->source.subsys.u.scsi.unit,
- dev->readonly);
+ dev->readonly,
+ dev->shareable);
if (!scsi)
goto done;
diff --git a/src/util/virscsi.c b/src/util/virscsi.c
index 7aca9e6..7462f9d 100644
--- a/src/util/virscsi.c
+++ b/src/util/virscsi.c
@@ -58,6 +58,7 @@ struct _virSCSIDevice {
const char *used_by; /* name of the domain using this dev */
bool readonly;
+ bool shareable;
};
struct _virSCSIDeviceList {
@@ -185,7 +186,8 @@ virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly)
+ bool readonly,
+ bool shareable)
{
virSCSIDevicePtr dev, ret = NULL;
char *sg = NULL;
@@ -201,6 +203,7 @@ virSCSIDeviceNew(const char *adapter,
dev->target = target;
dev->unit = unit;
dev->readonly = readonly;
+ dev->shareable= shareable;
Need a space prior to the "=" (dev->shareable = shareable)
ACK with the adjustment.
if (!(sg = virSCSIDeviceGetSgName(adapter, bus, target, unit)))
goto cleanup;
@@ -311,6 +314,12 @@ virSCSIDeviceGetReadonly(virSCSIDevicePtr dev)
return dev->readonly;
}
+bool
+virSCSIDeviceGetShareable(virSCSIDevicePtr dev)
+{
+ return dev->shareable;
+}
+
int
virSCSIDeviceFileIterate(virSCSIDevicePtr dev,
virSCSIDeviceFileActor actor,
diff --git a/src/util/virscsi.h b/src/util/virscsi.h
index cce5df4..84890be 100644
--- a/src/util/virscsi.h
+++ b/src/util/virscsi.h
@@ -46,7 +46,8 @@ virSCSIDevicePtr virSCSIDeviceNew(const char *adapter,
unsigned int bus,
unsigned int target,
unsigned int unit,
- bool readonly);
+ bool readonly,
+ bool shareable);
void virSCSIDeviceFree(virSCSIDevicePtr dev);
void virSCSIDeviceSetUsedBy(virSCSIDevicePtr dev, const char *name);
@@ -57,6 +58,7 @@ unsigned int virSCSIDeviceGetBus(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetTarget(virSCSIDevicePtr dev);
unsigned int virSCSIDeviceGetUnit(virSCSIDevicePtr dev);
bool virSCSIDeviceGetReadonly(virSCSIDevicePtr dev);
+bool virSCSIDeviceGetShareable(virSCSIDevicePtr dev);
/*
* Callback that will be invoked once for each file
8:45 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
It doesn't make sense to fail if the SCSI host device is specified
as "shareable" explicitly between domains (NB, it works if and only
if the device is specified as "shareable" for *all* domains,
otherwise it fails).
Also don't try to add the device to the activeScsiHostdevs list if
it's already there.
---
src/qemu/qemu_hostdev.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..8536499 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -1120,22 +1120,25 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
const char *other_name = virSCSIDeviceGetUsedBy(tmp);
- if (other_name)
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is in use by domain %s"),
- virSCSIDeviceGetName(tmp), other_name);
- else
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is already in use"),
- virSCSIDeviceGetName(tmp));
- goto error;
- }
-
- virSCSIDeviceSetUsedBy(scsi, name);
- VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
+ if (!(virSCSIDeviceGetShareable(scsi) &&
+ virSCSIDeviceGetShareable(tmp))) {
+ if (other_name)
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("SCSI device %s is in use by domain %s"),
+ virSCSIDeviceGetName(tmp), other_name);
+ else
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("SCSI device %s is already in use"),
+ virSCSIDeviceGetName(tmp));
+ goto error;
+ }
+ } else {
+ virSCSIDeviceSetUsedBy(scsi, name);
+ VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
- if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
- goto error;
+ if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
+ goto error;
+ }
}
virObjectUnlock(driver->activeScsiHostdevs);
--
1.8.1.4
9:54 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 01/02/2014 09:45 AM, Osier Yang wrote:
It doesn't make sense to fail if the SCSI host device is
specified
as "shareable" explicitly between domains (NB, it works if and only
if the device is specified as "shareable" for *all* domains,
otherwise it fails).
Also don't try to add the device to the activeScsiHostdevs list if
it's already there.
---
src/qemu/qemu_hostdev.c | 33 ++++++++++++++++++---------------
1 file changed, 18 insertions(+), 15 deletions(-)
diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
index 86a463a..8536499 100644
--- a/src/qemu/qemu_hostdev.c
+++ b/src/qemu/qemu_hostdev.c
@@ -1120,22 +1120,25 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
const char *other_name = virSCSIDeviceGetUsedBy(tmp);
- if (other_name)
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is in use by domain %s"),
- virSCSIDeviceGetName(tmp), other_name);
- else
- virReportError(VIR_ERR_OPERATION_INVALID,
- _("SCSI device %s is already in use"),
- virSCSIDeviceGetName(tmp));
- goto error;
- }
-
- virSCSIDeviceSetUsedBy(scsi, name);
- VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
+ if (!(virSCSIDeviceGetShareable(scsi) &&
+ virSCSIDeviceGetShareable(tmp))) {
+ if (other_name)
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("SCSI device %s is in use by domain
%s"),
+ virSCSIDeviceGetName(tmp), other_name);
+ else
+ virReportError(VIR_ERR_OPERATION_INVALID,
+ _("SCSI device %s is already in use"),
+ virSCSIDeviceGetName(tmp));
+ goto error;
+ }
+ } else {
+ virSCSIDeviceSetUsedBy(scsi, name);
+ VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
- if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
- goto error;
+ if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
+ goto error;
+ }
Something doesn't feel right here...
Prior code:
if find device on some other drivers active list
fail
set name of who device is used by
add device to host active list
(NOTE: will error if found on list already)
New code
if find device on some other drivers active list
if both devices not marked shareable
fail
else
set name of who device is used by
add device to host active list
So now theoretically with a shareable device, the device could be on the
active list and it's OK to use it because it's marked shareable.
So my question/issue becomes what to do with the "dev->used_by" (eg,
virSCSIDeviceSetUsedBy() result) value. In fact, since we're now
shareable it seems to me that we'd need to keep this up to date even
going so far as to clear usage it when virSCSIDeviceListDel() is called.
Should the list become a "list" of those using it? The first domain
that claims usage could eventually remove their usage, then what? It
would be strange to report domain X has the device in use if the domain
is down or doesn't exist. Use cscope and check the callers/users of
"used_by" to see what my concern is.
Remember prior to this point in time we cannot share, so it didn't
matter. However, from this point on since we can share we need to keep
track of who has it, right? What becomes even more interesting perhaps
is the impact on migration (if that's even possible with a shared hostdev).
John
}
virObjectUnlock(driver->activeScsiHostdevs);
5:17 a.m.
New subject: [libvirt] [PATCH 2/2] qemu: Don't fail if the SCSI host device is shareable between domains
On 06/01/14 23:54, John Ferlan wrote:
On 01/02/2014 09:45 AM, Osier Yang wrote:
> It doesn't make sense to fail if the SCSI host device is specified
> as "shareable" explicitly between domains (NB, it works if and only
> if the device is specified as "shareable" for *all* domains,
> otherwise it fails).
>
> Also don't try to add the device to the activeScsiHostdevs list if
> it's already there.
> ---
> src/qemu/qemu_hostdev.c | 33 ++++++++++++++++++---------------
> 1 file changed, 18 insertions(+), 15 deletions(-)
>
> diff --git a/src/qemu/qemu_hostdev.c b/src/qemu/qemu_hostdev.c
> index 86a463a..8536499 100644
> --- a/src/qemu/qemu_hostdev.c
> +++ b/src/qemu/qemu_hostdev.c
> @@ -1120,22 +1120,25 @@ qemuPrepareHostdevSCSIDevices(virQEMUDriverPtr driver,
> if ((tmp = virSCSIDeviceListFind(driver->activeScsiHostdevs, scsi))) {
> const char *other_name = virSCSIDeviceGetUsedBy(tmp);
>
> - if (other_name)
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("SCSI device %s is in use by domain
%s"),
> - virSCSIDeviceGetName(tmp), other_name);
> - else
> - virReportError(VIR_ERR_OPERATION_INVALID,
> - _("SCSI device %s is already in use"),
> - virSCSIDeviceGetName(tmp));
> - goto error;
> - }
> -
> - virSCSIDeviceSetUsedBy(scsi, name);
> - VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
> + if (!(virSCSIDeviceGetShareable(scsi) &&
> + virSCSIDeviceGetShareable(tmp))) {
> + if (other_name)
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("SCSI device %s is in use by domain
%s"),
> + virSCSIDeviceGetName(tmp), other_name);
> + else
> + virReportError(VIR_ERR_OPERATION_INVALID,
> + _("SCSI device %s is already in use"),
> + virSCSIDeviceGetName(tmp));
> + goto error;
> + }
> + } else {
> + virSCSIDeviceSetUsedBy(scsi, name);
> + VIR_DEBUG("Adding %s to activeScsiHostdevs",
virSCSIDeviceGetName(scsi));
>
> - if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> - goto error;
> + if (virSCSIDeviceListAdd(driver->activeScsiHostdevs, scsi) < 0)
> + goto error;
> + }
Something doesn't feel right here...
Prior code:
if find device on some other drivers active list
fail
set name of who device is used by
add device to host active list
(NOTE: will error if found on list already)
New code
if find device on some other drivers active list
if both devices not marked shareable
fail
else
set name of who device is used by
add device to host active list
So now theoretically with a shareable device, the device could be on the
active list and it's OK to use it because it's marked shareable.
Yes, as long as the later domain(s) specify the device as shareable
too.
So my question/issue becomes what to do with the "dev->used_by" (eg,
virSCSIDeviceSetUsedBy() result) value. In fact, since we're now
shareable it seems to me that we'd need to keep this up to date even
going so far as to clear usage it when virSCSIDeviceListDel() is called.
Should the list become a "list" of those using it?
/Alarm....
Yes, thanks for pointing it out...
The first domain
that claims usage could eventually remove their usage, then what? It
would be strange to report domain X has the device in use if the domain
is down or doesn't exist.
It actually won't report error. Since the "dev->used_by" is freed
along with the domain destroying. E.g.
% start domain A with sg7 as shareable.
% start domain B with sg7 as shareable
% Destroy domain A
% start domain C with sg7 as non-shareable.
Starting Domain C is expected to fail, but it will succeed.
I will post the v2.
Use cscope and check the callers/users of
"used_by" to see what my concern is.
Remember prior to this point in time we cannot share, so it didn't
matter. However, from this point on since we can share we need to keep
track of who has it, right? What becomes even more interesting perhaps
is the impact on migration (if that's even possible with a shared hostdev).
It should be fine for migration, as long as we make the "used_by"
correct.
Regards,
Osier
3974
days inactive
3979
days old
5 comments
2 participants
participants (2)
-
John Ferlan -
Osier Yang