[libvirt] [PATCH 0/3] Address a couple of mpath/Multipath storage pool issues
My thought was to combine patch 2 and 3 unless it was felt that disallowing more than one Multipath pool to defined at one time (patch 3) was unnecessary For patch 1, I did some searching and came across the following to describe why /dev/mpath is no longer valid: http://comments.gmane.org/gmane.linux.kernel.device-mapper.devel/13864 I kept /dev/mpath as a valid option only just in case there's some strange back-compat issue or some platform where /dev/mpath is preferred/used instead of /dev/mapper. John Ferlan (3): mpath: Update path in CheckPool function docs: Clarify the Multipath target path value usage mpath: Don't allow more than one mpath pool at a time docs/formatstorage.html.in | 2 ++ docs/storage.html.in | 3 ++- src/conf/storage_conf.c | 3 +++ src/storage/storage_backend_mpath.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-) -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1230664 Per the devmapper docs, use "/dev/mapper" or "/dev/dm-n" in order to determine if a device is under control of DM Multipath. So add "/dev/mapper" to the virFileExists, leaving the "/dev/mpath" as a "legacy" option since it appears for a while it was the preferred mechanism, but is no longer maintained Signed-off-by: John Ferlan <jferlan@redhat.com> --- src/storage/storage_backend_mpath.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/storage/storage_backend_mpath.c b/src/storage/storage_backend_mpath.c index 971408a..ca9a62f 100644 --- a/src/storage/storage_backend_mpath.c +++ b/src/storage/storage_backend_mpath.c @@ -248,7 +248,8 @@ static int virStorageBackendMpathCheckPool(virStoragePoolObjPtr pool ATTRIBUTE_UNUSED, bool *isActive) { - *isActive = virFileExists("/dev/mpath"); + *isActive = virFileExists("/dev/mapper") || + virFileExists("/dev/mpath"); return 0; } -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1232606 As described in the storage.html.in, a Multipath pool ignores the target element in favor of the default target mapping of /dev/mapper. Indicate so for formatstorage.html.in as well. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/formatstorage.html.in | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docs/formatstorage.html.in b/docs/formatstorage.html.in index b6f4361..a60e05e 100644 --- a/docs/formatstorage.html.in +++ b/docs/formatstorage.html.in @@ -401,6 +401,8 @@ guaranteed stable across reboots, since they are allocated on demand. It is preferable to use a stable location such as one of the <code>/dev/disk/by-{path|id|uuid|label}</code> locations. + For a Multipath pool (type <code>mpath</code>), the provided + value is ignored and the default value of "/dev/mapper" is used. <span class="since">Since 0.4.1</span> </dd> <dt><code>permissions</code></dt> -- 2.1.0
https://bugzilla.redhat.com/show_bug.cgi?id=1232606 Since an mpath pool contains all the Multipath devices on a host, allowing more than one defined on a host at a time should be disallowed under the policy of disallowing duplicate source pools for the host. Signed-off-by: John Ferlan <jferlan@redhat.com> --- docs/storage.html.in | 3 ++- src/conf/storage_conf.c | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/storage.html.in b/docs/storage.html.in index 0b467d5..6c8222a 100644 --- a/docs/storage.html.in +++ b/docs/storage.html.in @@ -505,7 +505,8 @@ <h2><a name="StorageBackendMultipath">Multipath pools</a></h2> <p> This provides a pool that contains all the multipath devices on the - host. Volume creating is not supported via the libvirt APIs. + host. Therefore, only one Multipath pool may be configured per host. + Volume creating is not supported via the libvirt APIs. The target element is actually ignored, but one is required to appease the libvirt XML parser.<br/> <br/> diff --git a/src/conf/storage_conf.c b/src/conf/storage_conf.c index 4bbed4f..971f5c1 100644 --- a/src/conf/storage_conf.c +++ b/src/conf/storage_conf.c @@ -2560,6 +2560,9 @@ virStoragePoolSourceFindDuplicate(virConnectPtr conn, matchpool = pool; break; case VIR_STORAGE_POOL_MPATH: + /* Only one mpath pool is valid per host */ + matchpool = pool; + break; case VIR_STORAGE_POOL_RBD: case VIR_STORAGE_POOL_LAST: break; -- 2.1.0
On 06/24/2015 09:06 AM, John Ferlan wrote:
My thought was to combine patch 2 and 3 unless it was felt that disallowing more than one Multipath pool to defined at one time (patch 3) was unnecessary
For patch 1, I did some searching and came across the following to describe why /dev/mpath is no longer valid:
http://comments.gmane.org/gmane.linux.kernel.device-mapper.devel/13864
I kept /dev/mpath as a valid option only just in case there's some strange back-compat issue or some platform where /dev/mpath is preferred/used instead of /dev/mapper.
John Ferlan (3): mpath: Update path in CheckPool function docs: Clarify the Multipath target path value usage mpath: Don't allow more than one mpath pool at a time
docs/formatstorage.html.in | 2 ++ docs/storage.html.in | 3 ++- src/conf/storage_conf.c | 3 +++ src/storage/storage_backend_mpath.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
ping... another simple pair Tks - John
On 24.06.2015 15:06, John Ferlan wrote:
My thought was to combine patch 2 and 3 unless it was felt that disallowing more than one Multipath pool to defined at one time (patch 3) was unnecessary
For patch 1, I did some searching and came across the following to describe why /dev/mpath is no longer valid:
http://comments.gmane.org/gmane.linux.kernel.device-mapper.devel/13864
I kept /dev/mpath as a valid option only just in case there's some strange back-compat issue or some platform where /dev/mpath is preferred/used instead of /dev/mapper.
John Ferlan (3): mpath: Update path in CheckPool function docs: Clarify the Multipath target path value usage mpath: Don't allow more than one mpath pool at a time
docs/formatstorage.html.in | 2 ++ docs/storage.html.in | 3 ++- src/conf/storage_conf.c | 3 +++ src/storage/storage_backend_mpath.c | 3 ++- 4 files changed, 9 insertions(+), 2 deletions(-)
ACK and safe for the freeze. Michal
participants (2)
-
John Ferlan -
Michal Privoznik