[libvirt] [PATCH] virfile: added GPFS as shared fs

From: dmichelotto <diego.michelotto@cnaf.infn.it> Added GPFS as shared file system recognized during live migration security checks. GPFS is 'IBM General Parallel File System' also called 'IBM Spectrum Scale' BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528 Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it> --- src/util/virfile.c | 9 ++++++++- src/util/virfile.h | 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 +++++ tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/util/virfile.c b/src/util/virfile.c index 271bf5e796..615c287104 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -3468,6 +3468,9 @@ int virFilePrintf(FILE *fp, const char *msg, ...) # ifndef CEPH_SUPER_MAGIC # define CEPH_SUPER_MAGIC 0x00C36400 # endif +# ifndef GPFS_SUPER_MAGIC +# define GPFS_SUPER_MAGIC 0x47504653 +# endif # define PROC_MOUNTS "/proc/mounts" @@ -3613,6 +3616,9 @@ virFileIsSharedFSType(const char *path, if ((fstypes & VIR_FILE_SHFS_CEPH) && (f_type == CEPH_SUPER_MAGIC)) return 1; + if ((fstypes & VIR_FILE_SHFS_GPFS) && + (f_type == GPFS_SUPER_MAGIC)) + return 1; return 0; } @@ -3776,7 +3782,8 @@ int virFileIsSharedFS(const char *path) VIR_FILE_SHFS_AFS | VIR_FILE_SHFS_SMB | VIR_FILE_SHFS_CIFS | - VIR_FILE_SHFS_CEPH); + VIR_FILE_SHFS_CEPH | + VIR_FILE_SHFS_GPFS); } diff --git a/src/util/virfile.h b/src/util/virfile.h index be612af770..0b946fe1ca 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -221,6 +221,7 @@ enum { VIR_FILE_SHFS_SMB = (1 << 4), VIR_FILE_SHFS_CIFS = (1 << 5), VIR_FILE_SHFS_CEPH = (1 << 6), + VIR_FILE_SHFS_GPFS = (1 << 7), }; int virFileIsSharedFSType(const char *path, int fstypes) ATTRIBUTE_NONNULL(1); diff --git a/tests/virfiledata/mounts3.txt b/tests/virfiledata/mounts3.txt index 68eded048c..4377e5d471 100644 --- a/tests/virfiledata/mounts3.txt +++ b/tests/virfiledata/mounts3.txt @@ -35,3 +35,4 @@ host:/gv0 /gluster fuse.glusterfs rw 0 0 root@host:/tmp/mkdir /gluster/sshfs fuse.sshfs rw 0 0 192.168.0.1:/ceph/data /ceph ceph rw,noatime,name=cephfs,secret=<hidden>,acl,wsize=16777216 0 0 192.168.0.1,192.168.0.2,192.168.0.3:/ceph/data2 /ceph/multi ceph rw,noatime,name=cephfs,secret=<hidden>,acl,wsize=16777216 0 0 +gpfs_data /gpfs/data gpfs rw,relatime 0 0 diff --git a/tests/virfilemock.c b/tests/virfilemock.c index 499135d773..89e14c5b67 100644 --- a/tests/virfilemock.c +++ b/tests/virfilemock.c @@ -89,6 +89,9 @@ setmntent(const char *filename, const char *type) #ifndef CEPH_SUPER_MAGIC # define CEPH_SUPER_MAGIC 0x00c36400 #endif +#ifndef GPFS_SUPER_MAGIC +# define GPFS_SUPER_MAGIC 0x47504653 +#endif static int @@ -137,6 +140,8 @@ statfs_mock(const char *mtab, ftype = FUSE_SUPER_MAGIC; } else if (STRPREFIX(mb.mnt_type, "ceph")) { ftype = CEPH_SUPER_MAGIC; + } else if (STRPREFIX(mb.mnt_type, "gpfs")) { + ftype = GPFS_SUPER_MAGIC; } else { /* Everything else is EXT4. We don't care really for other paths. */ ftype = EXT4_SUPER_MAGIC; diff --git a/tests/virfiletest.c b/tests/virfiletest.c index f90c611ac4..e2bd4953ed 100644 --- a/tests/virfiletest.c +++ b/tests/virfiletest.c @@ -457,6 +457,7 @@ mymain(void) DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/some/symlink/file", true); DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/ceph/file", true); DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/ceph/multi/file", true); + DO_TEST_FILE_IS_SHARED_FS_TYPE("mounts3.txt", "/gpfs/data", true); return ret != 0 ? EXIT_FAILURE : EXIT_SUCCESS; } -- 2.20.1

On Mon, Feb 25, 2019 at 19:19:03 +0100, Diego Michelotto wrote:
From: dmichelotto <diego.michelotto@cnaf.infn.it>
With your permission I'll remove the above line when applying the patch so that your authorship of the patch shows up as: Author: Diego Michelotto <diego.michelotto@cnaf.infn.it> instead of Author: dmichelotto <diego.michelotto@cnaf.infn.it>
Added GPFS as shared file system recognized during live migration security checks.
GPFS is 'IBM General Parallel File System' also called 'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it> --- src/util/virfile.c | 9 ++++++++- src/util/virfile.h | 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 +++++ tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-)
One thing which might be worth exploring is whether GPFS is also migratable with cache != none and thus should be also added to virStorageFileIsClusterFS. ACK. Depending on the angle of view this can be viewed both as a bug and as a feature. Since we are feature freeze, is anybody objecting in pushing this right away? (obviously I'm not going to push it right now)

HI Peter
Il giorno 26 feb 2019, alle ore 09:08, Peter Krempa <pkrempa@redhat.com> ha scritto:
On Mon, Feb 25, 2019 at 19:19:03 +0100, Diego Michelotto wrote:
From: dmichelotto <diego.michelotto@cnaf.infn.it <mailto:diego.michelotto@cnaf.infn.it>>
With your permission I'll remove the above line when applying the patch so that your authorship of the patch shows up as:
Author: Diego Michelotto <diego.michelotto@cnaf.infn.it <mailto:diego.michelotto@cnaf.infn.it>>
instead of
Author: dmichelotto <diego.michelotto@cnaf.infn.it <mailto:diego.michelotto@cnaf.infn.it>>
Sure, you can change it.
Added GPFS as shared file system recognized during live migration security checks.
GPFS is 'IBM General Parallel File System' also called 'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it> --- src/util/virfile.c | 9 ++++++++- src/util/virfile.h | 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 +++++ tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-)
One thing which might be worth exploring is whether GPFS is also migratable with cache != none and thus should be also added to virStorageFileIsClusterFS.
GPFS stalls badly when a process issues a lot of fsyncs in a short timeframe. It is better if the VM I/O is not synchronous, with previous version of libvirt we are using writeback caching. If you prefer I’ll try to make a new patch, but let me know if it’s easier for you to add yourself GPFS to the virStorageFileIsClusterFS function.
ACK.
Depending on the angle of view this can be viewed both as a bug and as a feature. Since we are feature freeze, is anybody objecting in pushing this right away? (obviously I'm not going to push it right now)

On Tue, Feb 26, 2019 at 09:34:21 +0100, Diego Michelotto wrote:
HI Peter
Il giorno 26 feb 2019, alle ore 09:08, Peter Krempa <pkrempa@redhat.com> ha scritto:
On Mon, Feb 25, 2019 at 19:19:03 +0100, Diego Michelotto wrote:
From: dmichelotto <diego.michelotto@cnaf.infn.it <mailto:diego.michelotto@cnaf.infn.it>>
With your permission I'll remove the above line when applying the patch so that your authorship of the patch shows up as:
Author: Diego Michelotto <diego.michelotto@cnaf.infn.it <mailto:diego.michelotto@cnaf.infn.it>>
instead of
Author: dmichelotto <diego.michelotto@cnaf.infn.it <mailto:diego.michelotto@cnaf.infn.it>>
Sure, you can change it.
Added GPFS as shared file system recognized during live migration security checks.
GPFS is 'IBM General Parallel File System' also called 'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it> --- src/util/virfile.c | 9 ++++++++- src/util/virfile.h | 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 +++++ tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-)
One thing which might be worth exploring is whether GPFS is also migratable with cache != none and thus should be also added to virStorageFileIsClusterFS.
GPFS stalls badly when a process issues a lot of fsyncs in a short timeframe. It is better if the VM I/O is not synchronous, with previous version of libvirt we are using writeback caching.
If you prefer I’ll try to make a new patch, but let me know if it’s easier for you to add yourself GPFS to the virStorageFileIsClusterFS function.
It will be better to send this as a separate patch. The safety of migration with caching is a complex problem which is harder to prove that it's correct. That one will require a justification of it's own. I'll push this meanwhile.

On Tue, Feb 26, 2019 at 09:08:27AM +0100, Peter Krempa wrote:
On Mon, Feb 25, 2019 at 19:19:03 +0100, Diego Michelotto wrote:
From: dmichelotto <diego.michelotto@cnaf.infn.it>
With your permission I'll remove the above line when applying the patch so that your authorship of the patch shows up as:
Author: Diego Michelotto <diego.michelotto@cnaf.infn.it>
instead of
Author: dmichelotto <diego.michelotto@cnaf.infn.it>
Added GPFS as shared file system recognized during live migration security checks.
GPFS is 'IBM General Parallel File System' also called 'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it> --- src/util/virfile.c | 9 ++++++++- src/util/virfile.h | 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 +++++ tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-)
One thing which might be worth exploring is whether GPFS is also migratable with cache != none and thus should be also added to virStorageFileIsClusterFS.
ACK.
Depending on the angle of view this can be viewed both as a bug and as a feature.
Bug, since we consider this a blocker for migration. Also, probably a regression introduced by commit ed11e9cd95bd9cae6cdfab14ae0936930bbb63e6 qemuMigrationSrcIsSafe: Check local storage more thoroughly
Since we are feature freeze, is anybody objecting in pushing this right away? (obviously I'm not going to push it right now)
I object to calling this a feature. Feel free to postpone if you deem it too risky or invasive. Jano

On Tue, Feb 26, 2019 at 09:08:27AM +0100, Peter Krempa wrote:
On Mon, Feb 25, 2019 at 19:19:03 +0100, Diego Michelotto wrote:
From: dmichelotto <diego.michelotto@cnaf.infn.it>
With your permission I'll remove the above line when applying the patch so that your authorship of the patch shows up as:
Author: Diego Michelotto <diego.michelotto@cnaf.infn.it>
instead of
Author: dmichelotto <diego.michelotto@cnaf.infn.it>
Added GPFS as shared file system recognized during live migration security checks.
GPFS is 'IBM General Parallel File System' also called 'IBM Spectrum Scale'
BUG: https://bugzilla.redhat.com/show_bug.cgi?id=1679528
Signed-off-by: Diego Michelotto <diego.michelotto@cnaf.infn.it> --- src/util/virfile.c | 9 ++++++++- src/util/virfile.h | 1 + tests/virfiledata/mounts3.txt | 1 + tests/virfilemock.c | 5 +++++ tests/virfiletest.c | 1 + 5 files changed, 16 insertions(+), 1 deletion(-)
One thing which might be worth exploring is whether GPFS is also migratable with cache != none and thus should be also added to virStorageFileIsClusterFS.
ACK.
Depending on the angle of view this can be viewed both as a bug and as a feature. Since we are feature freeze, is anybody objecting in pushing this right away? (obviously I'm not going to push it right now)
I can see it both ways, but regardless I think this is safe enough to push during freeze. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
participants (4)
-
Daniel P. Berrangé
-
Diego Michelotto
-
Ján Tomko
-
Peter Krempa