2:02 a.m.
If two or more disks are mapped to the same image file, operating
on these disks at the same time may corrupt data stored in the
image file.
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
src/conf/domain_conf.h | 2 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 6 ++++++
4 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1b02c25..93a081d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5455,6 +5455,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
if (!disk)
goto error;
+ if (virDomainDiskConflict(disk, def)) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"),
+ disk->src,
+ _("is already mapped to another device, "
+ "skip this device."));
+ continue;
+ }
def->disks[def->ndisks++] = disk;
}
VIR_FREE(nodes);
@@ -9088,3 +9096,17 @@ cleanup:
return ret;
}
+
+bool virDomainDiskConflict(virDomainDiskDefPtr disk, virDomainDefPtr def)
+{
+ int i;
+
+ if (!disk->src)
+ return false;
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (STREQ(disk->src, def->disks[i]->src))
+ return true;
+ }
+ return false;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9f595d6..78b2f95 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1350,6 +1350,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
bool ignoreOpenFailure,
virDomainDiskDefPathIterator iter,
void *opaque);
+bool virDomainDiskConflict(virDomainDiskDefPtr disk,
+ virDomainDefPtr def);
typedef const char* (*virLifecycleToStringFunc)(int type);
typedef int (*virLifecycleFromStringFunc)(const char *type);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b4b6c63..17e2ec4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -237,6 +237,7 @@ virDomainDeviceTypeToString;
virDomainDiskBusTypeToString;
virDomainDiskCacheTypeFromString;
virDomainDiskCacheTypeToString;
+virDomainDiskConflict;
virDomainDiskDefAssignAddress;
virDomainDiskDefForeachPath;
virDomainDiskDefFree;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6f296c9..6de08d3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4058,6 +4058,12 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
+ if (virDomainDiskConflict(dev->data.disk, vm->def)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"), dev->data.disk->src,
+ _("is already mapped to another device."));
+ break;
+ }
if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
ret = qemuDomainAttachUsbMassstorageDevice(driver, vm,
dev->data.disk, qemuCaps);
--
1.7.3.1
2:20 a.m.
At 03/24/2011 03:02 PM, Hu Tao Write:
If two or more disks are mapped to the same image file, operating
on these disks at the same time may corrupt data stored in the
image file.
If two disks are mapped to the same image and these two disks are read
only, operating on these two disks at the same is OK. Should we allow
this behavior?
---
src/conf/domain_conf.c | 22 ++++++++++++++++++++++
src/conf/domain_conf.h | 2 ++
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 6 ++++++
4 files changed, 31 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1b02c25..93a081d 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5455,6 +5455,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
if (!disk)
goto error;
+ if (virDomainDiskConflict(disk, def)) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"),
+ disk->src,
+ _("is already mapped to another device, "
+ "skip this device."));
+ continue;
+ }
def->disks[def->ndisks++] = disk;
}
VIR_FREE(nodes);
@@ -9088,3 +9096,17 @@ cleanup:
return ret;
}
+
+bool virDomainDiskConflict(virDomainDiskDefPtr disk, virDomainDefPtr def)
+{
+ int i;
+
+ if (!disk->src)
+ return false;
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (STREQ(disk->src, def->disks[i]->src))
If disk->src is '/var/lib/libvirt/images/test.img' and
def->disk[i]->src is '/var/libv//libvirt/images/test.img',
they are the same images, but STREQ can not work.
Comparing device id and inode number is a better way to check whether
two images are the same.
+ return true;
+ }
+ return false;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9f595d6..78b2f95 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1350,6 +1350,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
bool ignoreOpenFailure,
virDomainDiskDefPathIterator iter,
void *opaque);
+bool virDomainDiskConflict(virDomainDiskDefPtr disk,
+ virDomainDefPtr def);
typedef const char* (*virLifecycleToStringFunc)(int type);
typedef int (*virLifecycleFromStringFunc)(const char *type);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b4b6c63..17e2ec4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -237,6 +237,7 @@ virDomainDeviceTypeToString;
virDomainDiskBusTypeToString;
virDomainDiskCacheTypeFromString;
virDomainDiskCacheTypeToString;
+virDomainDiskConflict;
virDomainDiskDefAssignAddress;
virDomainDiskDefForeachPath;
virDomainDiskDefFree;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6f296c9..6de08d3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4058,6 +4058,12 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
+ if (virDomainDiskConflict(dev->data.disk, vm->def)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"), dev->data.disk->src,
+ _("is already mapped to another device."));
+ break;
+ }
if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
ret = qemuDomainAttachUsbMassstorageDevice(driver, vm,
dev->data.disk, qemuCaps);
3:46 a.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
If two or more disks are mapped to the same image file, operating
on these disks at the same time may corrupt data stored in the
image file.
changes:
v2:
- allow it for read-only disks
- compare source files by inode number
---
src/conf/domain_conf.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 2 +
src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 6 +++++
4 files changed, 63 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1b02c25..b43dc4a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5455,6 +5455,14 @@ static virDomainDefPtr virDomainDefParseXML(virCapsPtr caps,
if (!disk)
goto error;
+ if (virDomainDiskConflict(disk, def)) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"),
+ disk->src,
+ _("is already mapped to another device, "
+ "skip this device."));
+ continue;
+ }
def->disks[def->ndisks++] = disk;
}
VIR_FREE(nodes);
@@ -9088,3 +9096,49 @@ cleanup:
return ret;
}
+
+bool virDomainDiskConflict(virDomainDiskDefPtr disk, virDomainDefPtr def)
+{
+ struct stat stat1, stat2;
+ int i;
+
+ if (!disk->src)
+ return false;
+
+ if (stat(disk->src, &stat1)) {
+ if (errno != ENOENT) {
+ /* Can't stat file, for safety treate it as conflicted */
+ return true;
+ }
+ }
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (disk->readonly && def->disks[i]->readonly)
+ continue;
+
+ if (stat(def->disks[i]->src, &stat2)) {
+ if (errno != ENOENT) {
+ /* Can't stat file, shouldn't happen but for safety treate
+ * it as conflicted */
+ return true;
+ }
+ }
+
+ if (S_ISREG(stat1.st_mode) && S_ISREG(stat2.st_mode)
+ && (stat1.st_ino == stat2.st_ino)
+ && (stat1.st_dev == stat2.st_dev)) {
+ return true;
+ }
+
+ if (S_ISCHR(stat1.st_mode) && S_ISCHR(stat2.st_mode)
+ && (stat1.st_rdev == stat2.st_rdev)) {
+ return true;
+ }
+
+ if (S_ISBLK(stat1.st_mode) && S_ISBLK(stat2.st_mode)
+ && (stat1.st_rdev == stat2.st_rdev)) {
+ return true;
+ }
+ }
+ return false;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9f595d6..78b2f95 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1350,6 +1350,8 @@ int virDomainDiskDefForeachPath(virDomainDiskDefPtr disk,
bool ignoreOpenFailure,
virDomainDiskDefPathIterator iter,
void *opaque);
+bool virDomainDiskConflict(virDomainDiskDefPtr disk,
+ virDomainDefPtr def);
typedef const char* (*virLifecycleToStringFunc)(int type);
typedef int (*virLifecycleFromStringFunc)(const char *type);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b4b6c63..17e2ec4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -237,6 +237,7 @@ virDomainDeviceTypeToString;
virDomainDiskBusTypeToString;
virDomainDiskCacheTypeFromString;
virDomainDiskCacheTypeToString;
+virDomainDiskConflict;
virDomainDiskDefAssignAddress;
virDomainDiskDefForeachPath;
virDomainDiskDefFree;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6f296c9..6de08d3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4058,6 +4058,12 @@ static int qemudDomainAttachDevice(virDomainPtr dom,
break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
+ if (virDomainDiskConflict(dev->data.disk, vm->def)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"), dev->data.disk->src,
+ _("is already mapped to another device."));
+ break;
+ }
if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
ret = qemuDomainAttachUsbMassstorageDevice(driver, vm,
dev->data.disk, qemuCaps);
--
1.7.3.1
--
Thanks,
Hu Tao
2011/03/24
6:05 a.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On Thu, 24 Mar 2011 16:46:14 +0800
Hu Tao <hutao(a)cn.fujitsu.com> wrote:
If two or more disks are mapped to the same image file, operating
on these disks at the same time may corrupt data stored in the
image file.
changes:
v2:
- allow it for read-only disks
- compare source files by inode number
It doesn't seem that virDomainDiskConflict observes the "shared"
property of a disk definition.
If all the disks are marked as shared, we should say there is no
conflict.
---
src/conf/domain_conf.c | 54
++++++++++++++++++++++++++++++++++++++++++++++
src/conf/domain_conf.h | 2 + src/libvirt_private.syms | 1 +
src/qemu/qemu_driver.c | 6 +++++
4 files changed, 63 insertions(+), 0 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
index 1b02c25..b43dc4a 100644
--- a/src/conf/domain_conf.c
+++ b/src/conf/domain_conf.c
@@ -5455,6 +5455,14 @@ static virDomainDefPtr
virDomainDefParseXML(virCapsPtr caps, if (!disk)
goto error;
+ if (virDomainDiskConflict(disk, def)) {
+ virDomainReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"),
+ disk->src,
+ _("is already mapped to another
device, "
+ "skip this device."));
+ continue;
+ }
def->disks[def->ndisks++] = disk;
}
VIR_FREE(nodes);
@@ -9088,3 +9096,49 @@ cleanup:
return ret;
}
+
+bool virDomainDiskConflict(virDomainDiskDefPtr disk, virDomainDefPtr
def) +{
+ struct stat stat1, stat2;
+ int i;
+
+ if (!disk->src)
+ return false;
+
+ if (stat(disk->src, &stat1)) {
+ if (errno != ENOENT) {
+ /* Can't stat file, for safety treate it as conflicted */
+ return true;
+ }
+ }
+
+ for (i = 0; i < def->ndisks; i++) {
+ if (disk->readonly && def->disks[i]->readonly)
+ continue;
+
+ if (stat(def->disks[i]->src, &stat2)) {
+ if (errno != ENOENT) {
+ /* Can't stat file, shouldn't happen but for safety
treate
+ * it as conflicted */
+ return true;
+ }
+ }
+
+ if (S_ISREG(stat1.st_mode) && S_ISREG(stat2.st_mode)
+ && (stat1.st_ino == stat2.st_ino)
+ && (stat1.st_dev == stat2.st_dev)) {
+ return true;
+ }
+
+ if (S_ISCHR(stat1.st_mode) && S_ISCHR(stat2.st_mode)
+ && (stat1.st_rdev == stat2.st_rdev)) {
+ return true;
+ }
+
+ if (S_ISBLK(stat1.st_mode) && S_ISBLK(stat2.st_mode)
+ && (stat1.st_rdev == stat2.st_rdev)) {
+ return true;
+ }
+ }
+ return false;
+}
diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
index 9f595d6..78b2f95 100644
--- a/src/conf/domain_conf.h
+++ b/src/conf/domain_conf.h
@@ -1350,6 +1350,8 @@ int
virDomainDiskDefForeachPath(virDomainDiskDefPtr disk, bool
ignoreOpenFailure, virDomainDiskDefPathIterator iter,
void *opaque);
+bool virDomainDiskConflict(virDomainDiskDefPtr disk,
+ virDomainDefPtr def);
typedef const char* (*virLifecycleToStringFunc)(int type);
typedef int (*virLifecycleFromStringFunc)(const char *type);
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms
index b4b6c63..17e2ec4 100644
--- a/src/libvirt_private.syms
+++ b/src/libvirt_private.syms
@@ -237,6 +237,7 @@ virDomainDeviceTypeToString;
virDomainDiskBusTypeToString;
virDomainDiskCacheTypeFromString;
virDomainDiskCacheTypeToString;
+virDomainDiskConflict;
virDomainDiskDefAssignAddress;
virDomainDiskDefForeachPath;
virDomainDiskDefFree;
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index 6f296c9..6de08d3 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -4058,6 +4058,12 @@ static int
qemudDomainAttachDevice(virDomainPtr dom, break;
case VIR_DOMAIN_DISK_DEVICE_DISK:
+ if (virDomainDiskConflict(dev->data.disk, vm->def)) {
+ qemuReportError(VIR_ERR_INTERNAL_ERROR, "%s %s %s",
+ _("source"), dev->data.disk->src,
+ _("is already mapped to another
device."));
+ break;
+ }
if (dev->data.disk->bus == VIR_DOMAIN_DISK_BUS_USB) {
ret = qemuDomainAttachUsbMassstorageDevice(driver,
vm, dev->data.disk, qemuCaps);
1:26 a.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On Thu, Mar 24, 2011 at 11:05:08AM +0000, Nathan Baum wrote:
On Thu, 24 Mar 2011 16:46:14 +0800
Hu Tao <hutao(a)cn.fujitsu.com> wrote:
> If two or more disks are mapped to the same image file, operating
> on these disks at the same time may corrupt data stored in the
> image file.
>
> changes:
>
> v2:
>
> - allow it for read-only disks
> - compare source files by inode number
>
It doesn't seem that virDomainDiskConflict observes the "shared"
property of a disk definition.
If all the disks are marked as shared, we should say there is no
conflict.
Does the "shared" property mean virtual disks can be shared between
domains or the source of virtual disk can be shared between different
virtual disks? If it is the former, then the operation of sharing is
based on virtual disks, thus no need to care about this case. But if
it is the latter, we indeed have to check for the "shared" property.
--
Thanks,
Hu Tao
4:18 a.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On Fri, 25 Mar 2011 14:26:16 +0800
Hu Tao <hutao(a)cn.fujitsu.com> wrote:
On Thu, Mar 24, 2011 at 11:05:08AM +0000, Nathan Baum wrote:
> On Thu, 24 Mar 2011 16:46:14 +0800
> Hu Tao <hutao(a)cn.fujitsu.com> wrote:
>
> > If two or more disks are mapped to the same image file, operating
> > on these disks at the same time may corrupt data stored in the
> > image file.
> >
> > changes:
> >
> > v2:
> >
> > - allow it for read-only disks
> > - compare source files by inode number
> >
>
> It doesn't seem that virDomainDiskConflict observes the "shared"
> property of a disk definition.
>
> If all the disks are marked as shared, we should say there is no
> conflict.
Does the "shared" property mean virtual disks can be shared between
domains or the source of virtual disk can be shared between different
virtual disks? If it is the former, then the operation of sharing is
based on virtual disks, thus no need to care about this case. But if
it is the latter, we indeed have to check for the "shared" property.
I don't believe libvirt exposes a mechanism for sharing virtual devices
between domains. The only functions I know for adding devices accept
an XML description of the device to add, and I don't believe there is a
way to reference a disk that has been defined on another domain.
Assuming that's true, I don't think the distinction you're asking
about is meaningful. If disks can't be shared, then sharedness must
apply to sources to be at all meaningful. But I'm happy to be proven
wrong. :)
My current workflow is that I create domain XML for each domain, which,
when the storage is shared, contains identical <disk> stanza with
<shareable/> elements. Naturally, I'd like to continue using that
workflow.
4:45 p.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On 03/24/2011 02:46 AM, Hu Tao wrote:
If two or more disks are mapped to the same image file, operating
on these disks at the same time may corrupt data stored in the
image file.
changes:
v2:
- allow it for read-only disks
- compare source files by inode number
+
+ if (stat(disk->src, &stat1)) {
+ if (errno != ENOENT) {
+ /* Can't stat file, for safety treate it as conflicted */
s/treate/treat/
Won't this will fail on root-squash NFS from qemu:///system? (Or does
root-squash meant that root can still stat() but just not open() a file?)
Overall, I'm worried that this patch is repeating some of danpb's bigger
efforts to integrate a sanlock disk contention avoidance [1]. If a
resource manager is properly hooked to all disks, then we can prevent
contention between domains (and not limit ourself to just single-domain
contention, as in this patch). On the other hand, this seems like an
easy enough check to do for a single domain whether or not we get the
sanlock code completed (that is, timing wise this looks like it could be
ready prior to 0.9.0 while Dan's work is bigger in scope and probably
missed the feature freeze for this month's release). So I'm not sure
whether to ack this.
[1] https://www.redhat.com/archives/libvir-list/2011-January/msg00963.html
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
6:22 p.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On 03/24/2011 02:45 PM, Eric Blake wrote:
On 03/24/2011 02:46 AM, Hu Tao wrote:
> If two or more disks are mapped to the same image file, operating
> on these disks at the same time may corrupt data stored in the
> image file.
>
> changes:
>
> v2:
>
> - allow it for read-only disks
> - compare source files by inode number
>
> +
> + if (stat(disk->src, &stat1)) {
> + if (errno != ENOENT) {
> + /* Can't stat file, for safety treate it as conflicted */
s/treate/treat/
Won't this will fail on root-squash NFS from qemu:///system? (Or does
root-squash meant that root can still stat() but just not open() a file?)
This won't work for network disks, which aren't files. To check
for network disk conflicts, you'd need to check that whether any
host and port are the same as well. This won't be perfect, since
hosts and ports can be implicit or referred to by different
names, but it won't have false positives.
2:06 a.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On Thu, Mar 24, 2011 at 04:22:38PM -0700, Josh Durgin wrote:
On 03/24/2011 02:45 PM, Eric Blake wrote:
> On 03/24/2011 02:46 AM, Hu Tao wrote:
>> If two or more disks are mapped to the same image file, operating
>> on these disks at the same time may corrupt data stored in the
>> image file.
>>
>> changes:
>>
>> v2:
>>
>> - allow it for read-only disks
>> - compare source files by inode number
>>
>> +
>> + if (stat(disk->src, &stat1)) {
>> + if (errno != ENOENT) {
>> + /* Can't stat file, for safety treate it as conflicted */
>
> s/treate/treat/
>
> Won't this will fail on root-squash NFS from qemu:///system? (Or does
> root-squash meant that root can still stat() but just not open() a file?)
This won't work for network disks, which aren't files. To check
for network disk conflicts, you'd need to check that whether any
host and port are the same as well. This won't be perfect, since
hosts and ports can be implicit or referred to by different
names, but it won't have false positives.
Is there a perfect way to solve this problem? However I will try your
way first.
4:25 p.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On 03/25/2011 12:06 AM, Hu Tao wrote:
On Thu, Mar 24, 2011 at 04:22:38PM -0700, Josh Durgin wrote:
> This won't work for network disks, which aren't files. To check
> for network disk conflicts, you'd need to check that whether any
> host and port are the same as well. This won't be perfect, since
> hosts and ports can be implicit or referred to by different
> names, but it won't have false positives.
Is there a perfect way to solve this problem? However I will try your
way first.
I'm not sure there is a perfect way for all types of network
disks using the information libvirt has, since multiple
hostnames/ip addresses may refer to the same storage location.
A more robust solution would use a lower level interface to
implement locking. I'm not sure how this would work for nbd or
sheepdog, but for rbd the watch/notify mechanism in librados can
be used to do this. It sounds like this would be easier to do
later within Dan's lock manager framework.
It doesn't help much to only stop duplicate mappings on one
domain (or host) when your disks aren't local. If libvirt ends up
with a check that only works on one host or domain, skipping
network disks might be better than providing a false sense of
security.
2 a.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On Thu, Mar 24, 2011 at 03:45:26PM -0600, Eric Blake wrote:
On 03/24/2011 02:46 AM, Hu Tao wrote:
> If two or more disks are mapped to the same image file, operating
> on these disks at the same time may corrupt data stored in the
> image file.
>
> changes:
>
> v2:
>
> - allow it for read-only disks
> - compare source files by inode number
>
> +
> + if (stat(disk->src, &stat1)) {
> + if (errno != ENOENT) {
> + /* Can't stat file, for safety treate it as conflicted */
s/treate/treat/
Won't this will fail on root-squash NFS from qemu:///system? (Or does
root-squash meant that root can still stat() but just not open() a file?)
I think it won't. man stat says:
No permissions are required on the file itself (to stat it)
But needs 'r' bit to open().
Overall, I'm worried that this patch is repeating some of danpb's bigger
efforts to integrate a sanlock disk contention avoidance [1]. If a
resource manager is properly hooked to all disks, then we can prevent
contention between domains (and not limit ourself to just single-domain
contention, as in this patch). On the other hand, this seems like an
easy enough check to do for a single domain whether or not we get the
sanlock code completed (that is, timing wise this looks like it could be
ready prior to 0.9.0 while Dan's work is bigger in scope and probably
missed the feature freeze for this month's release). So I'm not sure
whether to ack this.
Once danpb's sanlock patch is ready we can easily migrate from the
method of this patch to the sanlock. But what is more important is
to protect users' data. Although the chance is rare that a user
attaches a disk using an image file that is already in use, but if
it happens and important data is lost, it will become a disaster
(especially for a productive environment)
[1] https://www.redhat.com/archives/libvir-list/2011-January/msg00963.html
--
Eric Blake eblake(a)redhat.com +1-801-349-2682
Libvirt virtualization library http://libvirt.org
9:35 a.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On 03/25/2011 03:00 AM, Hu Tao wrote:
On Thu, Mar 24, 2011 at 03:45:26PM -0600, Eric Blake wrote:
> On 03/24/2011 02:46 AM, Hu Tao wrote:
>> If two or more disks are mapped to the same image file, operating
>> on these disks at the same time may corrupt data stored in the
>> image file.
>>
>> changes:
>>
>> v2:
>>
>> - allow it for read-only disks
>> - compare source files by inode number
>>
>> +
>> + if (stat(disk->src,&stat1)) {
>> + if (errno != ENOENT) {
>> + /* Can't stat file, for safety treate it as conflicted */
> s/treate/treat/
>
> Won't this will fail on root-squash NFS from qemu:///system? (Or does
> root-squash meant that root can still stat() but just not open() a file?)
I think it won't. man stat says:
No permissions are required on the file itself (to stat it)
But needs 'r' bit to open().
root-squash means that root can only do what user "nobody" could do with
the filesystem. Although it doesn't need read access on the file itself,
stat() *will* fail if the current user isn't able to read the directory
containing the file being stat'ed. So, if user nobody doesn't have
access to the parent directory (ie if permissions are xx0, which is very
common), then root will not be able to stat the file - it will just
return -1.
To make this work properly for root-squash, you'll need to either 1)
fork a child that does setuid to the qemu user:group and does the stat()
(a real pain, since you'll need to pass the results back to the parent,
and can't reasonably log errors while in the child == not recommended),
or 2) use virFileOpenAs() to open the file as the qemu user
(virFileOpenAs() uses SCM_RIGHTS and a child process to do this), then
do fstat() of the file to get the info you need, and close it.
Keeping libvirt working properly with save images, disk images, and
pools on root-squash NFS is a never-ending battle (and can lead to an
intense hatred of NFS!) Basically any time new code is added that needs
to access any of these things, operation on root-squash is broken. Now
that we have virFileOpenAs(), it should be much more straightforward to
code so that root-squash scenarios keep working.
If anyone is adding some code that does anything with files on disk,
they don't have a root-squash NFS setup, and they want some testing to
make sure they haven't broken it, please point out the patch to me, and
I'll make the time to apply it locally and try it on my NFS setup. (I
know I should automatically just see it, but the volume on libvir-list
has increased *a lot* lately, and I frequently find myself several days
behind).
> Overall, I'm worried that this patch is repeating some of
danpb's bigger
> efforts to integrate a sanlock disk contention avoidance [1]. If a
> resource manager is properly hooked to all disks, then we can prevent
> contention between domains (and not limit ourself to just single-domain
> contention, as in this patch). On the other hand, this seems like an
> easy enough check to do for a single domain
My opinion is that it's probably much more likely that someone would
mistakenly use the same disk in two different domains (due to cut-paste
of XML) rather than using it twice in the same domain (that's much
easier to notice since the definitions are close to each other in the
same file).
So beyond the fact that this patch can't eliminate all erroneous
duplicate usage, it's not looking for the category that is most likely,
and thus it will probably have more an effect of providing a false sense
of security, rather than of catching any duplicate usage. That makes me
think this may actually do more harm than good.
> whether or not we get the
> sanlock code completed (that is, timing wise this looks like it could be
> ready prior to 0.9.0 while Dan's work is bigger in scope and probably
> missed the feature freeze for this month's release). So I'm not sure
> whether to ack this.
Assuming this patch (or something similar) was accepted, would anybody
(aside from those of us using upstream builds directly) ever get a
release that contained this patch, but didn't contain Dan's? If the
answer is no, then I think that's another reason to NACK this and wait
for the Dan's patch.
8:39 p.m.
New subject: [libvirt] [PATCHv2] Don't allow two or more disks to be mapped to the same image file
On Fri, Mar 25, 2011 at 10:35:13AM -0400, Laine Stump wrote:
On 03/25/2011 03:00 AM, Hu Tao wrote:
>On Thu, Mar 24, 2011 at 03:45:26PM -0600, Eric Blake wrote:
>>On 03/24/2011 02:46 AM, Hu Tao wrote:
>>>If two or more disks are mapped to the same image file, operating
>>>on these disks at the same time may corrupt data stored in the
>>>image file.
>>>
>>>changes:
>>>
>>>v2:
>>>
>>>- allow it for read-only disks
>>>- compare source files by inode number
>>>
>>>+
>>>+ if (stat(disk->src,&stat1)) {
>>>+ if (errno != ENOENT) {
>>>+ /* Can't stat file, for safety treate it as conflicted */
>>s/treate/treat/
>>
>>Won't this will fail on root-squash NFS from qemu:///system? (Or does
>>root-squash meant that root can still stat() but just not open() a file?)
>I think it won't. man stat says:
>
> No permissions are required on the file itself (to stat it)
>
>But needs 'r' bit to open().
root-squash means that root can only do what user "nobody" could do
with the filesystem. Although it doesn't need read access on the
file itself, stat() *will* fail if the current user isn't able to
read the directory containing the file being stat'ed. So, if user
nobody doesn't have access to the parent directory (ie if
permissions are xx0, which is very common), then root will not be
able to stat the file - it will just return -1.
To make this work properly for root-squash, you'll need to either 1)
fork a child that does setuid to the qemu user:group and does the
stat() (a real pain, since you'll need to pass the results back to
the parent, and can't reasonably log errors while in the child ==
not recommended), or 2) use virFileOpenAs() to open the file as the
qemu user (virFileOpenAs() uses SCM_RIGHTS and a child process to do
this), then do fstat() of the file to get the info you need, and
close it.
Keeping libvirt working properly with save images, disk images, and
pools on root-squash NFS is a never-ending battle (and can lead to
an intense hatred of NFS!) Basically any time new code is added that
needs to access any of these things, operation on root-squash is
broken. Now that we have virFileOpenAs(), it should be much more
straightforward to code so that root-squash scenarios keep working.
If anyone is adding some code that does anything with files on disk,
they don't have a root-squash NFS setup, and they want some testing
to make sure they haven't broken it, please point out the patch to
me, and I'll make the time to apply it locally and try it on my NFS
setup. (I know I should automatically just see it, but the volume on
libvir-list has increased *a lot* lately, and I frequently find
myself several days behind).
>>Overall, I'm worried that this patch is repeating some of danpb's bigger
>>efforts to integrate a sanlock disk contention avoidance [1]. If a
>>resource manager is properly hooked to all disks, then we can prevent
>>contention between domains (and not limit ourself to just single-domain
>>contention, as in this patch). On the other hand, this seems like an
>>easy enough check to do for a single domain
My opinion is that it's probably much more likely that someone would
mistakenly use the same disk in two different domains (due to
cut-paste of XML) rather than using it twice in the same domain
(that's much easier to notice since the definitions are close to
each other in the same file).
Agreed.
So beyond the fact that this patch can't eliminate all erroneous
duplicate usage, it's not looking for the category that is most
likely, and thus it will probably have more an effect of providing a
false sense of security, rather than of catching any duplicate
usage. That makes me think this may actually do more harm than good.
Agreed.
>>whether or not we get the
>>sanlock code completed (that is, timing wise this looks like it could be
>>ready prior to 0.9.0 while Dan's work is bigger in scope and probably
>>missed the feature freeze for this month's release). So I'm not sure
>>whether to ack this.
Assuming this patch (or something similar) was accepted, would
anybody (aside from those of us using upstream builds directly) ever
get a release that contained this patch, but didn't contain Dan's?
If the answer is no, then I think that's another reason to NACK this
and wait for the Dan's patch.
And thanks to all people involved in this thread for your applies. Now
it's clear that danp's snalock patch is a much better solution, so plz
skip this one.
--
Thanks,
Hu Tao
4988
days inactive
4992
days old
12 comments
6 participants
participants (6)
-
Eric Blake -
Hu Tao -
Josh Durgin -
Laine Stump -
Nathan Baum -
Wen Congyang