[libvirt] [PATCH 0/3] For 3.0: storage: fix regressions in storage driver
Few recent patches broke stuff. Revert/fix them. Peter Krempa (3): Revert "storage: For FS pool check for properly formatted target volume" Revert "storage: Validate the device formats at logical startup" storage: logical: Fix flawed logic with (NO)_OVERWRITE flags. src/storage/storage_backend_fs.c | 13 ++----------- src/storage/storage_backend_logical.c | 15 +-------------- 2 files changed, 3 insertions(+), 25 deletions(-) -- 2.11.0
The check does not work properly (crashes) with netfs filesystems and also checking that a device is not empty when attempting to mount a filesystem is not very usefull since the mount will fail anyways. As the code would improve only a very minor corner case I don't really see a reason to have this code at all. This code would also fail if libvirt is compiled without support for blkid and without parted. This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4. --- src/storage/storage_backend_fs.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f4341f32c..f0ef07b2f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -605,17 +605,8 @@ static int virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - const char *format = - virStoragePoolFormatFileSystemTypeToString(pool->def->source.format); - const char *path = pool->def->source.devices[0].path; - - if (pool->def->type == VIR_STORAGE_POOL_DIR) - return 0; - - if (!virStorageBackendDeviceIsEmpty(path, format, false)) - return -1; - - if (virStorageBackendFileSystemMount(pool) < 0) + if (pool->def->type != VIR_STORAGE_POOL_DIR && + virStorageBackendFileSystemMount(pool) < 0) return -1; return 0; -- 2.11.0
On 01/12/2017 09:24 AM, Peter Krempa wrote:
The check does not work properly (crashes) with netfs filesystems and also checking that a device is not empty when attempting to mount a filesystem is not very usefull since the mount will fail anyways.
As the code would improve only a very minor corner case I don't really see a reason to have this code at all.
This code would also fail if libvirt is compiled without support for blkid and without parted.
This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4. --- src/storage/storage_backend_fs.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Instead of reverting why not just fix the issue. John
diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f4341f32c..f0ef07b2f 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -605,17 +605,8 @@ static int virStorageBackendFileSystemStart(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - const char *format = - virStoragePoolFormatFileSystemTypeToString(pool->def->source.format); - const char *path = pool->def->source.devices[0].path; - - if (pool->def->type == VIR_STORAGE_POOL_DIR) - return 0; - - if (!virStorageBackendDeviceIsEmpty(path, format, false)) - return -1; - - if (virStorageBackendFileSystemMount(pool) < 0) + if (pool->def->type != VIR_STORAGE_POOL_DIR && + virStorageBackendFileSystemMount(pool) < 0) return -1;
return 0;
On Thu, Jan 12, 2017 at 09:36:01 -0500, John Ferlan wrote:
On 01/12/2017 09:24 AM, Peter Krempa wrote:
The check does not work properly (crashes) with netfs filesystems and also checking that a device is not empty when attempting to mount a filesystem is not very usefull since the mount will fail anyways.
As the code would improve only a very minor corner case I don't really see a reason to have this code at all.
This code would also fail if libvirt is compiled without support for blkid and without parted.
This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4. --- src/storage/storage_backend_fs.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Instead of reverting why not just fix the issue.
Because I think the whole check is pointless. It's valid only when mounting a local filesystem, and in that case basically the same check is done when mounting the filesystem by the kernel.
On 01/12/2017 09:41 AM, Peter Krempa wrote:
On Thu, Jan 12, 2017 at 09:36:01 -0500, John Ferlan wrote:
On 01/12/2017 09:24 AM, Peter Krempa wrote:
The check does not work properly (crashes) with netfs filesystems and also checking that a device is not empty when attempting to mount a filesystem is not very usefull since the mount will fail anyways.
As the code would improve only a very minor corner case I don't really see a reason to have this code at all.
This code would also fail if libvirt is compiled without support for blkid and without parted.
This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4. --- src/storage/storage_backend_fs.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Instead of reverting why not just fix the issue.
Because I think the whole check is pointless. It's valid only when mounting a local filesystem, and in that case basically the same check is done when mounting the filesystem by the kernel.
Here's the difference between the check and removing the check (not withstanding the no PARTED and no BLKID available)... Create a pool using ext4, start it - life is happy. Destroy the pool. Define a pool using xfs. Start it w/ my change: # virsh pool-start fs error: Failed to start pool fs error: Storage pool already built: Device '/dev/sde' formatted cannot overwrite using 'xfs', requires build --overwrite w/o my change (e.g. if the change is reverted): # virsh pool-start fs error: Failed to start pool fs error: internal error: Child process (/usr/bin/mount -t xfs /dev/sde /home/vm-images/fs) unexpected exit status 32: mount: wrong fs type, bad option, bad superblock on /dev/sde, missing codepage or helper program, or other error In some cases useful info is found in syslog - try dmesg | tail or so. ---- I personally like the former, but I do understand/agree that the latter will also work. John
On Thu, Jan 12, 2017 at 19:45:52 -0500, John Ferlan wrote:
Here's the difference between the check and removing the check (not withstanding the no PARTED and no BLKID available)...
Create a pool using ext4, start it - life is happy. Destroy the pool. Define a pool using xfs. Start it
w/ my change:
# virsh pool-start fs error: Failed to start pool fs error: Storage pool already built: Device '/dev/sde' formatted cannot overwrite using 'xfs', requires build --overwrite
This error message doesn't make any sense to me. You are starting a pool, but libvirt is complaining that it is already built (well, it must be, otherwise you cannot start it) and suggesting to use --overwrite, which is specific to virsh (API users would be confused) and gives the feeling the command is trying to format the device while it only tries to mount it. In other words, the check makes sense when a user tries to built (i.e., format) the pool, but it is pretty misleading here.
w/o my change (e.g. if the change is reverted):
# virsh pool-start fs error: Failed to start pool fs error: internal error: Child process (/usr/bin/mount -t xfs /dev/sde /home/vm-images/fs) unexpected exit status 32: mount: wrong fs type, bad option, bad superblock on /dev/sde,
On the other hand, this error is pretty clear. Jirka
On Thu, Jan 12, 2017 at 19:45:52 -0500, John Ferlan wrote:
On 01/12/2017 09:41 AM, Peter Krempa wrote:
On Thu, Jan 12, 2017 at 09:36:01 -0500, John Ferlan wrote:
On 01/12/2017 09:24 AM, Peter Krempa wrote:
The check does not work properly (crashes) with netfs filesystems and also checking that a device is not empty when attempting to mount a filesystem is not very usefull since the mount will fail anyways.
As the code would improve only a very minor corner case I don't really see a reason to have this code at all.
This code would also fail if libvirt is compiled without support for blkid and without parted.
This reverts commit a11fd69735e6951cda9bf256d8e423696a441aa4. --- src/storage/storage_backend_fs.c | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-)
Instead of reverting why not just fix the issue.
Because I think the whole check is pointless. It's valid only when mounting a local filesystem, and in that case basically the same check is done when mounting the filesystem by the kernel.
Here's the difference between the check and removing the check (not withstanding the no PARTED and no BLKID available)...
Create a pool using ext4, start it - life is happy. Destroy the pool. Define a pool using xfs. Start it
w/ my change:
# virsh pool-start fs error: Failed to start pool fs error: Storage pool already built: Device '/dev/sde' formatted cannot overwrite using 'xfs', requires build --overwrite
I think that the error code and the message are in fact misleading here. You are starting the pool not building it, so both the message comming from the error code and the free form string state nonsensical facts.
w/o my change (e.g. if the change is reverted):
# virsh pool-start fs error: Failed to start pool fs error: internal error: Child process (/usr/bin/mount -t xfs /dev/sde /home/vm-images/fs) unexpected exit status 32: mount: wrong fs type, bad option, bad superblock on /dev/sde, missing codepage or helper program, or other error
In some cases useful info is found in syslog - try dmesg | tail or so.
While this is a bit too verbose, but in my opinion describes the problem better. Peter
The check is pointless since LVM is capable to detect it's own members and the check is flawed as it would fail if neither libblkid nor parted is installed. We don't really need to babysit LVM in this way. This reverts commit cb38b6cbc7e35e7ee92a7f54828f21261227d17a. --- src/storage/storage_backend_logical.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 85e080bf9..6a6720e22 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -743,19 +743,6 @@ static int virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - size_t i; - - /* Let's make sure the pool's devices are properly formatted */ - for (i = 0; i < pool->def->source.ndevice; i++) { - const char *path = pool->def->source.devices[i].path; - - /* The blkid FS and Part probing code doesn't know "lvm2" (this - * pool's only format type), but it does know "LVM2_member", so - * we'll pass that here */ - if (!virStorageBackendDeviceIsEmpty(path, "LVM2_member", false)) - return -1; - } - /* Let's make sure that the pool's name matches the pvs output and * that the pool's source devices match the pvs output. */ -- 2.11.0
On 01/12/2017 09:24 AM, Peter Krempa wrote:
The check is pointless since LVM is capable to detect it's own members and the check is flawed as it would fail if neither libblkid nor parted is installed.
We don't really need to babysit LVM in this way.
This reverts commit cb38b6cbc7e35e7ee92a7f54828f21261227d17a. --- src/storage/storage_backend_logical.c | 13 ------------- 1 file changed, 13 deletions(-)
Ambivalent on this one - the virStorageBackendDeviceIsEmpty was added here and in the previous patch for FS mainly because Disk pools had a similar check. Since each uses some sort of device backend onto which a specific format (as defined by the pool) should be written - it just seem logical to check. You call it babysitting - I call it validation. John
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 85e080bf9..6a6720e22 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -743,19 +743,6 @@ static int virStorageBackendLogicalStartPool(virConnectPtr conn ATTRIBUTE_UNUSED, virStoragePoolObjPtr pool) { - size_t i; - - /* Let's make sure the pool's devices are properly formatted */ - for (i = 0; i < pool->def->source.ndevice; i++) { - const char *path = pool->def->source.devices[i].path; - - /* The blkid FS and Part probing code doesn't know "lvm2" (this - * pool's only format type), but it does know "LVM2_member", so - * we'll pass that here */ - if (!virStorageBackendDeviceIsEmpty(path, "LVM2_member", false)) - return -1; - } - /* Let's make sure that the pool's name matches the pvs output and * that the pool's source devices match the pvs output. */
Commit f573f84eb7 introduced flawed logic which would cause a regression in creating of lvm volumes when neither libblkid nor parted are installed. Fix the logic so that it triggers only if NO_OVERWRITE was specified explicitly. --- src/storage/storage_backend_logical.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 6a6720e22..77fc0d3c2 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -778,7 +778,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, /* The blkid FS and Part probing code doesn't know "lvm2" (this * pool's only format type), but it does know "LVM2_member", so * we'll pass that here */ - if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) && + if ((flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE) && !virStorageBackendDeviceIsEmpty(path, "LVM2_member", true)) goto cleanup; -- 2.11.0
On 01/12/2017 09:24 AM, Peter Krempa wrote:
Commit f573f84eb7 introduced flawed logic which would cause a regression in creating of lvm volumes when neither libblkid nor parted are installed.
Fix the logic so that it triggers only if NO_OVERWRITE was specified explicitly. --- src/storage/storage_backend_logical.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
So if no flag is provided, this change would essentially allow an overwrite of data on the target device. If the target host doesn't have blkid and/or parted, then not only does this code fail, so does the fs and disk code - even prior to any of the patches from the series. For both disk/fs - if someone doesn't have blkid/parted, then they must provide the overwrite flag to indicate they know what they're doing; otherwise, we just fail because we either cannot or did not validate that the device had nothing on it and we don't want to overwrite something that may be important. What makes logical "special" so that we just ignore the check? History? - that is since we've never checked, thus we have to keep that going forward? From a data integrity viewpoint, that doesn't feel right. John
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 6a6720e22..77fc0d3c2 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -778,7 +778,7 @@ virStorageBackendLogicalBuildPool(virConnectPtr conn ATTRIBUTE_UNUSED, /* The blkid FS and Part probing code doesn't know "lvm2" (this * pool's only format type), but it does know "LVM2_member", so * we'll pass that here */ - if (!(flags & VIR_STORAGE_POOL_BUILD_OVERWRITE) && + if ((flags & VIR_STORAGE_POOL_BUILD_NO_OVERWRITE) && !virStorageBackendDeviceIsEmpty(path, "LVM2_member", true)) goto cleanup;
On Thu, Jan 12, 2017 at 10:12:39 -0500, John Ferlan wrote:
On 01/12/2017 09:24 AM, Peter Krempa wrote:
Commit f573f84eb7 introduced flawed logic which would cause a regression in creating of lvm volumes when neither libblkid nor parted are installed.
Fix the logic so that it triggers only if NO_OVERWRITE was specified explicitly. --- src/storage/storage_backend_logical.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
So if no flag is provided, this change would essentially allow an overwrite of data on the target device.
If the target host doesn't have blkid and/or parted, then not only does this code fail, so does the fs and disk code - even prior to any of the
Disk code is guaranteed to have at least 'parted'. It would not be built otherwise
patches from the series. For both disk/fs - if someone doesn't have blkid/parted, then they must provide the overwrite flag to indicate they know what they're doing; otherwise, we just fail because we either cannot or did not validate that the device had nothing on it and we don't want to overwrite something that may be important.
Umm, you know that the code checks only wheter there's a different LVM physical volume? If there's a filesystem or whatever else it still nukes it.
What makes logical "special" so that we just ignore the check? History?
I was motivated by history and the strange tristateness of the flags. Also one of the big problems were the name and semantics of 'virStorageBackendDeviceIsEmpty'. The negative semantics and name containing "Empty" distracted me. At any rate, the usage as is makes sense, so I'll drop this patch. On the other hand I figured out that the usage of parted together with blkid is broken: In virStorageBackendPARTEDValidLabel which calls 'parted' looks for 'Partition table' and then feeds it to 'virStoragePoolFormatDiskTypeFromString' This type implements the following values: VIR_ENUM_IMPL(virStoragePoolFormatDisk, VIR_STORAGE_POOL_DISK_LAST, "unknown", "dos", "dvh", "gpt", "mac", "bsd", "pc98", "sun", "lvm2") The code paths in : virStorageBackendLogicalStartPool virStorageBackendLogicalBuildPool virStorageBackendMakeFileSystem virStorageBackendFileSystemStart feed values from 'virStoragePoolFormatFileSystem' or the value 'LVM2_member'. The virStoragePoolFormatFileSystem implements the following strings: VIR_ENUM_IMPL(virStoragePoolFormatFileSystem, VIR_STORAGE_POOL_FS_LAST, "auto", "ext2", "ext3", "ext4", "ufs", "iso9660", "udf", "gfs", "gfs2", "vfat", "hfs+", "xfs", "ocfs2") There's no intersection of those two thus if you don't have blkid installed the code always fails to match the type. virStorageBackendPARTEDValidLabel is not a replacement for virStorageBackendBLKIDFindEmpty. The patchset wrongly joins the cases for disks, where the disklabel (partition table) is relevant, with other cases where the type of the filesystem itself is relevant. The above facts also result into the fact that when liblkid is not installed starting of logical and filesystem pools will fail all the time. This is the justification for the two revert patches. In addition the intergation of virStorageBackendBLKIDFindFS and virStorageBackendBLKIDFindPart in virStorageBackendBLKIDFindEmpty is weird. There are two separate cases: 1) you need to look up the disklabel (partition table) type 2) you need to look up the filesystem type I don't see any sane reason for combining the two.
On 01/12/2017 01:08 PM, Peter Krempa wrote:
On Thu, Jan 12, 2017 at 10:12:39 -0500, John Ferlan wrote:
On 01/12/2017 09:24 AM, Peter Krempa wrote:
Commit f573f84eb7 introduced flawed logic which would cause a regression in creating of lvm volumes when neither libblkid nor parted are installed.
Fix the logic so that it triggers only if NO_OVERWRITE was specified explicitly. --- src/storage/storage_backend_logical.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
So if no flag is provided, this change would essentially allow an overwrite of data on the target device.
If the target host doesn't have blkid and/or parted, then not only does this code fail, so does the fs and disk code - even prior to any of the
Disk code is guaranteed to have at least 'parted'. It would not be built otherwise
True... and the Disk code ended up being the "model" to a degree.
patches from the series. For both disk/fs - if someone doesn't have blkid/parted, then they must provide the overwrite flag to indicate they know what they're doing; otherwise, we just fail because we either cannot or did not validate that the device had nothing on it and we don't want to overwrite something that may be important.
Umm, you know that the code checks only wheter there's a different LVM physical volume? If there's a filesystem or whatever else it still nukes it.
What makes logical "special" so that we just ignore the check? History?
I was motivated by history and the strange tristateness of the flags.
Also one of the big problems were the name and semantics of 'virStorageBackendDeviceIsEmpty'. The negative semantics and name containing "Empty" distracted me.
Well the "Probe" name was disliked by review and "Empty" while not perfect was deemed OK. Is there a name that you believe would be better?
At any rate, the usage as is makes sense, so I'll drop this patch.
On the other hand I figured out that the usage of parted together with blkid is broken:
In virStorageBackendPARTEDValidLabel which calls 'parted' looks for 'Partition table' and then feeds it to 'virStoragePoolFormatDiskTypeFromString'
This type implements the following values:
VIR_ENUM_IMPL(virStoragePoolFormatDisk, VIR_STORAGE_POOL_DISK_LAST, "unknown", "dos", "dvh", "gpt", "mac", "bsd", "pc98", "sun", "lvm2")
The code paths in : virStorageBackendLogicalStartPool virStorageBackendLogicalBuildPool virStorageBackendMakeFileSystem virStorageBackendFileSystemStart
feed values from 'virStoragePoolFormatFileSystem' or the value 'LVM2_member'. The virStoragePoolFormatFileSystem implements the following strings:
VIR_ENUM_IMPL(virStoragePoolFormatFileSystem, VIR_STORAGE_POOL_FS_LAST, "auto", "ext2", "ext3", "ext4", "ufs", "iso9660", "udf", "gfs", "gfs2", "vfat", "hfs+", "xfs", "ocfs2")
There's no intersection of those two thus if you don't have blkid installed the code always fails to match the type. virStorageBackendPARTEDValidLabel is not a replacement for virStorageBackendBLKIDFindEmpty.
The patchset wrongly joins the cases for disks, where the disklabel (partition table) is relevant, with other cases where the type of the filesystem itself is relevant.
The intent was being able to use BLKID if available instead of PARTED. Combining blkid (fsid/partid) and parted was a suggestion from a review of the FS overwrite issue. See "[C]" towards the end of the following: http://www.redhat.com/archives/libvir-list/2016-November/msg00858.html For the FS pool the BLKID should be the same... Being able to have a fall back to have PARTED run just in case there was something that PARTED knew about felt like a bonus. Being able to use BLKID for disk in order to determine if "something" was on the target device (e.g. ends up being the partfs calls) seemed like a better mechanism than spawning off PARTED and all that comes with that. Taking it the extra step for the logical backend seemed like a logical (sic) step to me. I'll need to think a bit longer on the rest of what you wrote and this is not something that doesn't need to hold up 3.0.0, so do what you must and I'll put this on a list of things to get back to.
The above facts also result into the fact that when liblkid is not installed starting of logical and filesystem pools will fail all the time. This is the justification for the two revert patches.
Simple enough to fix - return -2 if PARTED is not installed and then check in the caller for -2 and change to ret = 0. Although I do understand your point.
In addition the intergation of virStorageBackendBLKIDFindFS and virStorageBackendBLKIDFindPart in virStorageBackendBLKIDFindEmpty is weird.
There are two separate cases: 1) you need to look up the disklabel (partition table) type 2) you need to look up the filesystem type
I don't see any sane reason for combining the two.
It's a means to ensure you don't have something "unexpected" on the target device you're about to build/use for your pool. Before any of these patches, all FS would do is check for "known" FS types... Furthermore it would *filter* on the current type, so it would erroneously *pass* if the format type differed - thus allowing a build of "new" format type on some existing type. Before any of these patches, all DISK would do is use PARTED. Now it can use BLKID in order to make sure something isn't on the target device. And then LOGICAL did nothing - it just happily overwrote the first 512 bytes of the target device and ran pvcreate on it without checking for *anything*. John
On Thu, Jan 12, 2017 at 03:24:07PM +0100, Peter Krempa wrote:
Few recent patches broke stuff. Revert/fix them.
Peter Krempa (3): Revert "storage: For FS pool check for properly formatted target volume" Revert "storage: Validate the device formats at logical startup" storage: logical: Fix flawed logic with (NO)_OVERWRITE flags.
src/storage/storage_backend_fs.c | 13 ++----------- src/storage/storage_backend_logical.c | 15 +-------------- 2 files changed, 3 insertions(+), 25 deletions(-)
ACK series Jan
participants (4)
-
Jiri Denemark -
John Ferlan -
Ján Tomko -
Peter Krempa