2:29 p.m.
Adjusting how DASDs are handled as recently ID_* tags are also included
in the udev information which causes the problems reported by
https://issues.redhat.com/browse/RHEL-39497
Removing the filtering of offline ccw devices as these devices are
available in the system and also are used to set them online again. The
state information is made available in the ccw capability as an optional
state element.
Boris Fiuczynski (4):
nodedev: refactor storage type fixup
nodedev: improve DASD detection
nodedev: prevent invalid DASD node object creation
nodedev: add ccw device state and remove fencing
src/conf/node_device_conf.c | 24 +++++++++++++
src/conf/node_device_conf.h | 11 ++++++
src/conf/schemas/nodedev.rng | 8 +++++
src/node_device/node_device_udev.c | 56 +++++++++++++++++++++++++-----
4 files changed, 90 insertions(+), 9 deletions(-)
--
2.45.0
2:29 p.m.
New subject: [PATCH 1/4] nodedev: refactor storage type fixup
Refactor the storage type fixup into a reusable method.
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/node_device/node_device_udev.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 3b85db00da..15e31d522a 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -962,6 +962,21 @@ udevProcessDASD(struct udev_device *device,
}
+static int
+udevFixupStorageType(virNodeDeviceDef *def, const char *prefix, const char *subst)
+{
+ if (STRPREFIX(def->caps->data.storage.block, prefix)) {
+ def->caps->data.storage.drive_type = g_strdup(subst);
+ VIR_DEBUG("Found storage type '%s' for device with sysfs path
'%s'",
+ def->caps->data.storage.drive_type,
+ def->sysfs_path);
+ return 1;
+ }
+
+ return 0;
+}
+
+
/* This function exists to deal with the case in which a driver does
* not provide a device type in the usual place, but udev told us it's
* a storage device, and we can make a good guess at what kind of
@@ -994,13 +1009,8 @@ udevKludgeStorageType(virNodeDeviceDef *def)
def->sysfs_path);
for (i = 0; i < G_N_ELEMENTS(fixups); i++) {
- if (STRPREFIX(def->caps->data.storage.block, fixups[i].prefix)) {
- def->caps->data.storage.drive_type = g_strdup(fixups[i].subst);
- VIR_DEBUG("Found storage type '%s' for device with sysfs path
'%s'",
- def->caps->data.storage.drive_type,
- def->sysfs_path);
+ if (udevFixupStorageType(def, fixups[i].prefix, fixups[i].subst))
return 0;
- }
}
VIR_DEBUG("Could not determine storage type "
--
2.45.0
9:41 a.m.
New subject: [PATCH 1/4] nodedev: refactor storage type fixup
On 6/19/24 14:29, Boris Fiuczynski wrote:
Refactor the storage type fixup into a reusable method.
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/node_device/node_device_udev.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 3b85db00da..15e31d522a 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -962,6 +962,21 @@ udevProcessDASD(struct udev_device *device,
}
+static int
+udevFixupStorageType(virNodeDeviceDef *def, const char *prefix, const char *subst)
Nitpick, one argument per line please.
+{
+ if (STRPREFIX(def->caps->data.storage.block, prefix)) {
+ def->caps->data.storage.drive_type = g_strdup(subst);
+ VIR_DEBUG("Found storage type '%s' for device with sysfs path
'%s'",
+ def->caps->data.storage.drive_type,
+ def->sysfs_path);
+ return 1;
+ }
+
+ return 0;
+}
+
+
/* This function exists to deal with the case in which a driver does
* not provide a device type in the usual place, but udev told us it's
* a storage device, and we can make a good guess at what kind of
@@ -994,13 +1009,8 @@ udevKludgeStorageType(virNodeDeviceDef *def)
def->sysfs_path);
for (i = 0; i < G_N_ELEMENTS(fixups); i++) {
- if (STRPREFIX(def->caps->data.storage.block, fixups[i].prefix)) {
- def->caps->data.storage.drive_type = g_strdup(fixups[i].subst);
- VIR_DEBUG("Found storage type '%s' for device with sysfs path
'%s'",
- def->caps->data.storage.drive_type,
- def->sysfs_path);
+ if (udevFixupStorageType(def, fixups[i].prefix, fixups[i].subst))
return 0;
- }
}
VIR_DEBUG("Could not determine storage type "
Michal
2:29 p.m.
New subject: [PATCH 2/4] nodedev: improve DASD detection
In newer DASD driver versions the ID_TYPE tag is supported. This tag is
missing after a system reboot but when the ccw device is set offline and
online the tag is included. To fix this version independently we need to
check if devices detected as type disk is actually a DASD to maintain
the node object consistency and not end up with multiple node objects
for DASDs.
Resolves: https://issues.redhat.com/browse/RHEL-39497
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/node_device/node_device_udev.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 15e31d522a..c0e258fe9c 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1065,6 +1065,10 @@ udevProcessStorage(struct udev_device *device,
storage->drive_type = g_strdup("sd");
else if (udevKludgeStorageType(def) != 0)
goto cleanup;
+ } else {
+ /* A detected disk might be a DASD */
+ if (STREQ(def->caps->data.storage.drive_type, "disk"))
+ udevFixupStorageType(def, "/dev/dasd", "dasd");
}
if (STREQ(def->caps->data.storage.drive_type, "cd") ||
--
2.45.0
2:29 p.m.
New subject: [PATCH 3/4] nodedev: prevent invalid DASD node object creation
Prevent the creation of a new DASD node object when the device does not
exist.
Resolves: https://issues.redhat.com/browse/RHEL-39497
Reviewed-by: Marc Hartmayer <mhartmay(a)linux.ibm.com>
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/node_device/node_device_udev.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index c0e258fe9c..ad994ef0b2 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -958,6 +958,9 @@ udevProcessDASD(struct udev_device *device,
udevGetStringSysfsAttr(device, "device/uid", &storage->serial);
+ if (!storage->serial)
+ return -1;
+
return udevProcessDisk(device, def);
}
--
2.45.0
2:29 p.m.
New subject: [PATCH 4/4] nodedev: add ccw device state and remove fencing
Instead of fencing offline ccw devices add the state to the ccw
capability.
Resolves: https://issues.redhat.com/browse/RHEL-39497
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/conf/node_device_conf.c | 24 ++++++++++++++++++++++++
src/conf/node_device_conf.h | 11 +++++++++++
src/conf/schemas/nodedev.rng | 8 ++++++++
src/node_device/node_device_udev.c | 27 ++++++++++++++++++++++++---
4 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 416238bec1..08a89942ba 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -86,6 +86,12 @@ VIR_ENUM_IMPL(virNodeDevDRM,
"render",
);
+VIR_ENUM_IMPL(virNodeDevCCWState,
+ VIR_NODE_DEV_CCW_STATE_LAST,
+ "offline",
+ "online",
+);
+
static int
virNodeDevCapsDefParseString(const char *xpath,
xmlXPathContextPtr ctxt,
@@ -743,6 +749,10 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int
flags)
virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state);
break;
case VIR_NODE_DEV_CAP_CCW_DEV:
+ if (data->ccw_dev.state != VIR_NODE_DEV_CCW_STATE_LAST) {
+ const char *state =
virNodeDevCCWStateTypeToString(data->ccw_dev.state);
+ virBufferEscapeString(&buf,
"<state>%s</state>\n", state);
+ }
virNodeDeviceCapCCWDefFormat(&buf, data);
break;
case VIR_NODE_DEV_CAP_CSS_DEV:
@@ -1189,9 +1199,23 @@ virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
{
VIR_XPATH_NODE_AUTORESTORE(ctxt)
g_autofree virCCWDeviceAddress *ccw_addr = NULL;
+ g_autofree char *state = NULL;
+ int val;
ctxt->node = node;
+ /* state is optional */
+ ccw_dev->state = VIR_NODE_DEV_CCW_STATE_LAST;
+
+ if ((state = virXPathString("string(./state[1])", ctxt))) {
+ if ((val = virNodeDevCCWStateTypeFromString(state)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown state '%1$s' for
'%2$s'"), state, def->name);
+ return -1;
+ }
+ ccw_dev->state = val;
+ }
+
ccw_addr = g_new0(virCCWDeviceAddress, 1);
if (virNodeDevCCWDeviceAddressParseXML(ctxt, node, def->name, ccw_addr) < 0)
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 4b82636af7..32a8a5884b 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -112,6 +112,16 @@ typedef enum {
VIR_NODE_DEV_CAP_FLAG_CSS_MDEV = (1 << 0),
} virNodeDevCCWCapFlags;
+typedef enum {
+ /* Keep in sync with VIR_ENUM_IMPL in node_device_conf.c */
+ VIR_NODE_DEV_CCW_STATE_OFFLINE = 0,
+ VIR_NODE_DEV_CCW_STATE_ONLINE,
+
+ VIR_NODE_DEV_CCW_STATE_LAST
+} virNodeDevCCWStateType;
+
+VIR_ENUM_DECL(virNodeDevCCWState);
+
typedef enum {
VIR_NODE_DEV_CAP_FLAG_AP_MATRIX_MDEV = (1 << 0),
} virNodeDevAPMatrixCapFlags;
@@ -279,6 +289,7 @@ struct _virNodeDevCapCCW {
virMediatedDeviceType **mdev_types;
size_t nmdev_types;
virCCWDeviceAddress *channel_dev_addr;
+ virNodeDevCCWStateType state;
};
typedef struct _virNodeDevCapVDPA virNodeDevCapVDPA;
diff --git a/src/conf/schemas/nodedev.rng b/src/conf/schemas/nodedev.rng
index ff07313968..42a0cdcfd9 100644
--- a/src/conf/schemas/nodedev.rng
+++ b/src/conf/schemas/nodedev.rng
@@ -673,6 +673,14 @@
<attribute name="type">
<value>ccw</value>
</attribute>
+ <optional>
+ <element name="state">
+ <choice>
+ <value>online</value>
+ <value>offline</value>
+ </choice>
+ </element>
+ </optional>
<ref name="capccwaddress"/>
</define>
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index ad994ef0b2..9d11261b88 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1199,13 +1199,34 @@ udevGetCCWAddress(const char *sysfs_path,
static int
-udevProcessCCW(struct udev_device *device,
- virNodeDeviceDef *def)
+udevCCWGetState(struct udev_device *device,
+ virNodeDevCapData *data)
{
int online = 0;
+ if (udevGetIntSysfsAttr(device, "online", &online, 0) < 0 || online
< 0)
+ return -1;
+
+ switch (online) {
+ case VIR_NODE_DEV_CCW_STATE_OFFLINE:
+ case VIR_NODE_DEV_CCW_STATE_ONLINE:
+ data->ccw_dev.state = online;
+ break;
+ default:
+ data->ccw_dev.state = VIR_NODE_DEV_CCW_STATE_LAST;
+ break;
+ }
+
+ return 0;
+}
+
+
+static int
+udevProcessCCW(struct udev_device *device,
+ virNodeDeviceDef *def)
+{
/* process only online devices to keep the list sane */
- if (udevGetIntSysfsAttr(device, "online", &online, 0) < 0 || online
!= 1)
+ if (udevCCWGetState(device, &def->caps->data) < 0)
return -1;
if (udevGetCCWAddress(def->sysfs_path, &def->caps->data) < 0)
--
2.45.0
9:41 a.m.
New subject: [PATCH 4/4] nodedev: add ccw device state and remove fencing
On 6/19/24 14:29, Boris Fiuczynski wrote:
Instead of fencing offline ccw devices add the state to the ccw
capability.
Resolves: https://issues.redhat.com/browse/RHEL-39497
Signed-off-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com>
---
src/conf/node_device_conf.c | 24 ++++++++++++++++++++++++
src/conf/node_device_conf.h | 11 +++++++++++
src/conf/schemas/nodedev.rng | 8 ++++++++
src/node_device/node_device_udev.c | 27 ++++++++++++++++++++++++---
4 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index 416238bec1..08a89942ba 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -86,6 +86,12 @@ VIR_ENUM_IMPL(virNodeDevDRM,
"render",
);
+VIR_ENUM_IMPL(virNodeDevCCWState,
+ VIR_NODE_DEV_CCW_STATE_LAST,
+ "offline",
+ "online",
+);
+
static int
virNodeDevCapsDefParseString(const char *xpath,
xmlXPathContextPtr ctxt,
@@ -743,6 +749,10 @@ virNodeDeviceDefFormat(const virNodeDeviceDef *def, unsigned int
flags)
virNodeDeviceCapMdevDefFormat(&buf, data, inactive_state);
break;
case VIR_NODE_DEV_CAP_CCW_DEV:
+ if (data->ccw_dev.state != VIR_NODE_DEV_CCW_STATE_LAST) {
+ const char *state =
virNodeDevCCWStateTypeToString(data->ccw_dev.state);
+ virBufferEscapeString(&buf,
"<state>%s</state>\n", state);
+ }
virNodeDeviceCapCCWDefFormat(&buf, data);
break;
case VIR_NODE_DEV_CAP_CSS_DEV:
@@ -1189,9 +1199,23 @@ virNodeDevCapCCWParseXML(xmlXPathContextPtr ctxt,
{
VIR_XPATH_NODE_AUTORESTORE(ctxt)
g_autofree virCCWDeviceAddress *ccw_addr = NULL;
+ g_autofree char *state = NULL;
+ int val;
ctxt->node = node;
+ /* state is optional */
+ ccw_dev->state = VIR_NODE_DEV_CCW_STATE_LAST;
+
+ if ((state = virXPathString("string(./state[1])", ctxt))) {
+ if ((val = virNodeDevCCWStateTypeFromString(state)) < 0) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("unknown state '%1$s' for
'%2$s'"), state, def->name);
+ return -1;
+ }
+ ccw_dev->state = val;
+ }
+
ccw_addr = g_new0(virCCWDeviceAddress, 1);
if (virNodeDevCCWDeviceAddressParseXML(ctxt, node, def->name, ccw_addr) < 0)
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 4b82636af7..32a8a5884b 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -112,6 +112,16 @@ typedef enum {
VIR_NODE_DEV_CAP_FLAG_CSS_MDEV = (1 << 0),
} virNodeDevCCWCapFlags;
+typedef enum {
+ /* Keep in sync with VIR_ENUM_IMPL in node_device_conf.c */
This is not needed because our VIR_ENUM_* macros do a compile time
asserts to check if respective enums have the same number of members.
I know you copied this over from a pre-existing code but the comment
there is also unnecessary.
+ VIR_NODE_DEV_CCW_STATE_OFFLINE = 0,
+ VIR_NODE_DEV_CCW_STATE_ONLINE,
+
+ VIR_NODE_DEV_CCW_STATE_LAST
+} virNodeDevCCWStateType;
+
+VIR_ENUM_DECL(virNodeDevCCWState);
+
Michal
5:10 p.m.
Thank you, Boris.
Is the following test scenario sufficient to describe this change, are you aware of any
other scenarios that might be at risk of seeing a regression, maybe some scenario for
other storage devices like SCSI?
Given a DASD disk with identifier 0.0.4024 on subchannel 0.0.0030
When I deconfigure the device (chzdev -d 0.0.4024)
And I configure the device (chzdev -e 0.0.4204)
And I request the xml details of node devices (virsh nodedev-list --storage)
Then the DASD disk is listed
And its parent is a device with capability ccw and identifier 0.0.4024
And the parent of that device is another device with capability css and identifier
0.0.0030
5:51 p.m.
On 6/19/24 5:10 PM, smitterl(a)redhat.com wrote:
Thank you, Boris.
Is the following test scenario sufficient to describe this change, are you aware of any
other scenarios that might be at risk of seeing a regression, maybe some scenario for
other storage devices like SCSI?
Given a DASD disk with identifier 0.0.4024 on subchannel 0.0.0030
When I deconfigure the device (chzdev -d 0.0.4024)
And I configure the device (chzdev -e 0.0.4204)
And I request the xml details of node devices (virsh nodedev-list --storage)
I am
not aware of a "storage" option on command nodedev-list. My guess
is that your are referring to "nodedev-list --tree".
Then the DASD disk is listed
And its parent is a device with capability ccw and identifier 0.0.4024
And the parent of that device is another device with capability css and identifier
0.0.0030
The first three patches do not actually change existing behavior but
instead fix the problem described in the referenced issue.
When the ccw device is configured (online) the relationships are and
also should have been as you describe above.
Theses patches are not modifying SCSI storage device node behavior.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
12:24 p.m.
On 6/19/24 5:10 PM, smitterl(a)redhat.com wrote:
I am
not aware of a "storage" option on command nodedev-list. My guess
is that your are referring to "nodedev-list --tree".
I meant the
'--cap storage' option. But I see the '--tree' also works and it
additionally displays the hierarchy that my test usually would traverse by checking
dumpxml and there the parents.
The first three patches do not actually change existing behavior but
instead fix the problem described in the referenced issue.
When the ccw device is configured (online) the relationships are and
also should have been as you describe above.
Theses patches are not modifying SCSI storage device node behavior.
Understood.
I was able to confirm that with the scratch build Jiŕi Denemark provided
a) the device is listed only once
b) the device' parent is correctly set (ccw instead of css)
c) no critical product regression hit in our test suite tp-libvirt for test set
'virsh.nodedev_list,virsh.nodedev_dumpxml'
9:41 a.m.
On 6/19/24 14:29, Boris Fiuczynski wrote:
Adjusting how DASDs are handled as recently ID_* tags are also
included
in the udev information which causes the problems reported by
https://issues.redhat.com/browse/RHEL-39497
Removing the filtering of offline ccw devices as these devices are
available in the system and also are used to set them online again. The
state information is made available in the ccw capability as an optional
state element.
Boris Fiuczynski (4):
nodedev: refactor storage type fixup
nodedev: improve DASD detection
nodedev: prevent invalid DASD node object creation
nodedev: add ccw device state and remove fencing
src/conf/node_device_conf.c | 24 +++++++++++++
src/conf/node_device_conf.h | 11 ++++++
src/conf/schemas/nodedev.rng | 8 +++++
src/node_device/node_device_udev.c | 56 +++++++++++++++++++++++++-----
4 files changed, 90 insertions(+), 9 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and merged.
Michal
10:01 a.m.
On 6/20/24 9:41 AM, Michal Prívozník wrote:
On 6/19/24 14:29, Boris Fiuczynski wrote:
> Adjusting how DASDs are handled as recently ID_* tags are also included
> in the udev information which causes the problems reported by
> https://issues.redhat.com/browse/RHEL-39497
> Removing the filtering of offline ccw devices as these devices are
> available in the system and also are used to set them online again. The
> state information is made available in the ccw capability as an optional
> state element.
>
> Boris Fiuczynski (4):
> nodedev: refactor storage type fixup
> nodedev: improve DASD detection
> nodedev: prevent invalid DASD node object creation
> nodedev: add ccw device state and remove fencing
>
> src/conf/node_device_conf.c | 24 +++++++++++++
> src/conf/node_device_conf.h | 11 ++++++
> src/conf/schemas/nodedev.rng | 8 +++++
> src/node_device/node_device_udev.c | 56 +++++++++++++++++++++++++-----
> 4 files changed, 90 insertions(+), 9 deletions(-)
>
Reviewed-by: Michal Privoznik <mprivozn(a)redhat.com>
and merged.
Michal
Thanks, Michal.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Wolfgang Wendt
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
88
days inactive
89
days old
11 comments
3 participants
participants (3)
-
Boris Fiuczynski -
Michal Prívozník -
smitterl＠redhat.com