[1/2] conf: nodedev: Don't refresh host caps in testdriver

Message ID ba2150b8236347b7edb7a8e56c4af76798498d96.1519427518.git.crobinso@redhat.com
State New
Headers show
Series
  • test: Implement virConnectListAllNodeDevices
Related show

Commit Message

Cole Robinson Feb. 23, 2018, 11:16 p.m.
Add a 'testdriver' bool that we set for test_driver.c nodedevs
which will skip accessing host resources via virNodeDeviceUpdateCaps

Signed-off-by: Cole Robinson <crobinso@redhat.com>

---
 src/conf/node_device_conf.c | 3 +++
 src/conf/node_device_conf.h | 1 +
 src/test/test_driver.c      | 2 ++
 3 files changed, 6 insertions(+)

-- 
2.14.3

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Comments

John Ferlan March 2, 2018, 9:02 p.m. | #1
On 02/23/2018 06:16 PM, Cole Robinson wrote:
> Add a 'testdriver' bool that we set for test_driver.c nodedevs

> which will skip accessing host resources via virNodeDeviceUpdateCaps

> 

> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> ---

>  src/conf/node_device_conf.c | 3 +++

>  src/conf/node_device_conf.h | 1 +

>  src/test/test_driver.c      | 2 ++

>  3 files changed, 6 insertions(+)

> 

> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c

> index fd8f4e4a9..90c940f11 100644

> --- a/src/conf/node_device_conf.c

> +++ b/src/conf/node_device_conf.c

> @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)

>  {

>      virNodeDevCapsDefPtr cap = def->caps;

>  

> +    if (def->testdriver)

> +        return 0;

> +

>      while (cap) {

>          switch (cap->data.type) {

>          case VIR_NODE_DEV_CAP_SCSI_HOST:

> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h

> index 685ae3034..665f766e2 100644

> --- a/src/conf/node_device_conf.h

> +++ b/src/conf/node_device_conf.h

> @@ -316,6 +316,7 @@ struct _virNodeDeviceDef {

>      char *driver;                       /* optional driver name */

>      char *devnode;                      /* /dev path */

>      char **devlinks;                    /* /dev links */

> +    bool testdriver;                    /* if true, skip host checks */


Not sure this should be in virNodeDeviceDef... I think it should be in
virNodeDeviceObj. Yes, a bit more work, but I think cleaner. You'd need
to create an accessor function in order to set the flag from
test_driver. Then avoid calling virNodeDeviceUpdateCaps only from
virNodeDeviceMatch if the flag is set.

Also instead of "testdriver", how about "skipUpdateCaps" since the
purpose of this is to skip calling virNodeDeviceUpdateCaps?

At least that hides that this currently is only for the test driver.
Perhaps in the future there could be some other reason to not want to
update the caps for some specific definition after perhaps it's "known"
or "determined" that a specific update had occurred.

John

>      virNodeDevCapsDefPtr caps;          /* optional device capabilities */

>  };

>  

> diff --git a/src/test/test_driver.c b/src/test/test_driver.c

> index 043caa976..39784c9fa 100644

> --- a/src/test/test_driver.c

> +++ b/src/test/test_driver.c

> @@ -1165,6 +1165,7 @@ testParseNodedevs(testDriverPtr privconn,

>          if (!def)

>              goto error;

>  

> +        def->testdriver = true;

>          if (!(obj = virNodeDeviceObjListAssignDef(privconn->devs, def))) {

>              virNodeDeviceDefFree(def);

>              goto error;

> @@ -5565,6 +5566,7 @@ testNodeDeviceMockCreateVport(testDriverPtr driver,

>          caps = caps->next;

>      }

>  

> +    def->testdriver = true;

>      if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))

>          goto cleanup;

>      def = NULL;

> 


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Cole Robinson March 2, 2018, 9:36 p.m. | #2
On 03/02/2018 04:02 PM, John Ferlan wrote:
> 

> 

> On 02/23/2018 06:16 PM, Cole Robinson wrote:

>> Add a 'testdriver' bool that we set for test_driver.c nodedevs

>> which will skip accessing host resources via virNodeDeviceUpdateCaps

>>

>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>> ---

>>  src/conf/node_device_conf.c | 3 +++

>>  src/conf/node_device_conf.h | 1 +

>>  src/test/test_driver.c      | 2 ++

>>  3 files changed, 6 insertions(+)

>>

>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c

>> index fd8f4e4a9..90c940f11 100644

>> --- a/src/conf/node_device_conf.c

>> +++ b/src/conf/node_device_conf.c

>> @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)

>>  {

>>      virNodeDevCapsDefPtr cap = def->caps;

>>  

>> +    if (def->testdriver)

>> +        return 0;

>> +

>>      while (cap) {

>>          switch (cap->data.type) {

>>          case VIR_NODE_DEV_CAP_SCSI_HOST:

>> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h

>> index 685ae3034..665f766e2 100644

>> --- a/src/conf/node_device_conf.h

>> +++ b/src/conf/node_device_conf.h

>> @@ -316,6 +316,7 @@ struct _virNodeDeviceDef {

>>      char *driver;                       /* optional driver name */

>>      char *devnode;                      /* /dev path */

>>      char **devlinks;                    /* /dev links */

>> +    bool testdriver;                    /* if true, skip host checks */

> 

> Not sure this should be in virNodeDeviceDef... I think it should be in

> virNodeDeviceObj. Yes, a bit more work, but I think cleaner. You'd need

> to create an accessor function in order to set the flag from

> test_driver. Then avoid calling virNodeDeviceUpdateCaps only from

> virNodeDeviceMatch if the flag is set.

> 

> Also instead of "testdriver", how about "skipUpdateCaps" since the

> purpose of this is to skip calling virNodeDeviceUpdateCaps?

> 

> At least that hides that this currently is only for the test driver.

> Perhaps in the future there could be some other reason to not want to

> update the caps for some specific definition after perhaps it's "known"

> or "determined" that a specific update had occurred.


Okay, thanks for review. How about finding a way to remove UpdateCaps
from generic conf.c implementations instead? Seems wrong that something
functions needed to implement ListAllDevices touch host resources, I
don't think other objects work like that

Thanks,
Cole

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
John Ferlan March 2, 2018, 10:53 p.m. | #3
On 03/02/2018 04:36 PM, Cole Robinson wrote:
> On 03/02/2018 04:02 PM, John Ferlan wrote:

>>

>>

>> On 02/23/2018 06:16 PM, Cole Robinson wrote:

>>> Add a 'testdriver' bool that we set for test_driver.c nodedevs

>>> which will skip accessing host resources via virNodeDeviceUpdateCaps

>>>

>>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

>>> ---

>>>  src/conf/node_device_conf.c | 3 +++

>>>  src/conf/node_device_conf.h | 1 +

>>>  src/test/test_driver.c      | 2 ++

>>>  3 files changed, 6 insertions(+)

>>>

>>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c

>>> index fd8f4e4a9..90c940f11 100644

>>> --- a/src/conf/node_device_conf.c

>>> +++ b/src/conf/node_device_conf.c

>>> @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)

>>>  {

>>>      virNodeDevCapsDefPtr cap = def->caps;

>>>  

>>> +    if (def->testdriver)

>>> +        return 0;

>>> +

>>>      while (cap) {

>>>          switch (cap->data.type) {

>>>          case VIR_NODE_DEV_CAP_SCSI_HOST:

>>> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h

>>> index 685ae3034..665f766e2 100644

>>> --- a/src/conf/node_device_conf.h

>>> +++ b/src/conf/node_device_conf.h

>>> @@ -316,6 +316,7 @@ struct _virNodeDeviceDef {

>>>      char *driver;                       /* optional driver name */

>>>      char *devnode;                      /* /dev path */

>>>      char **devlinks;                    /* /dev links */

>>> +    bool testdriver;                    /* if true, skip host checks */

>>

>> Not sure this should be in virNodeDeviceDef... I think it should be in

>> virNodeDeviceObj. Yes, a bit more work, but I think cleaner. You'd need

>> to create an accessor function in order to set the flag from

>> test_driver. Then avoid calling virNodeDeviceUpdateCaps only from

>> virNodeDeviceMatch if the flag is set.

>>

>> Also instead of "testdriver", how about "skipUpdateCaps" since the

>> purpose of this is to skip calling virNodeDeviceUpdateCaps?

>>

>> At least that hides that this currently is only for the test driver.

>> Perhaps in the future there could be some other reason to not want to

>> update the caps for some specific definition after perhaps it's "known"

>> or "determined" that a specific update had occurred.

> 

> Okay, thanks for review. How about finding a way to remove UpdateCaps

> from generic conf.c implementations instead? Seems wrong that something

> functions needed to implement ListAllDevices touch host resources, I

> don't think other objects work like that

> 


The call to add update for export was a fairly recent, commit id
'd18feadc'. Seems that Erik Skultety has the most recent knowledge.
Hopefully he reads and chimes in...

The nodedev driver isn't like other drivers w/r/t being able to define
or create defs on your own. Instead you rely on udev (or gasp hal) in
order to provide "events" that would add, change, delete the device and
get it's capabilities.

It gets worse because even though an event is fired, it doesn't mean the
update has really occurred. I know mdevs and npiv scsi_host/target's are
afflicted by similar problems - udev says it's got an event, but some
other layer such as systemd is still filling in the data. For npiv
devices, I found that there was a possibility if the timing was just
right that the wwnn, wwpn, and fabric_wwn fields were set to 0 or
ffffffffffffffff, see bz 1319544 for some details. I did generate a
patch to work around it, but it wasn't accepted because it was a hack or
workaround essentially. I know I've reviewed a patch recently in the
mdev space for a somewhat similar problem.

I wonder if it would be better to have some sort of refresh logic that
would do the same/similar type magic at least with respect to getting
updates for the 4 types (SCSI_HOST, SCSI_TARGET, NET, PCI_DEV) that seem
to be lagging or needing to get the latest information. Ironically I
would half expect MDEV or MDEV_TYPES to be included in the need update
list if only because of the similar issues to npiv at least w/r/t
systemd interaction. In any case, I'm not sure there's a simple way to
"know" your data is out of date.

Perhaps the UpdateCaps call for Export isn't necessary, but I can see a
reason for it - that "automagic update" functionality. Just because it
doesn't work well for the test driver isn't perhaps a "good enough"
reason to just remove it.

I'm willing to be convinced otherwise though!

John

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Erik Skultety March 6, 2018, 4:16 p.m. | #4
On Fri, Mar 02, 2018 at 05:53:22PM -0500, John Ferlan wrote:
>

>

> On 03/02/2018 04:36 PM, Cole Robinson wrote:

> > On 03/02/2018 04:02 PM, John Ferlan wrote:

> >>

> >>

> >> On 02/23/2018 06:16 PM, Cole Robinson wrote:

> >>> Add a 'testdriver' bool that we set for test_driver.c nodedevs

> >>> which will skip accessing host resources via virNodeDeviceUpdateCaps

> >>>

> >>> Signed-off-by: Cole Robinson <crobinso@redhat.com>

> >>> ---

> >>>  src/conf/node_device_conf.c | 3 +++

> >>>  src/conf/node_device_conf.h | 1 +

> >>>  src/test/test_driver.c      | 2 ++

> >>>  3 files changed, 6 insertions(+)

> >>>

> >>> diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c

> >>> index fd8f4e4a9..90c940f11 100644

> >>> --- a/src/conf/node_device_conf.c

> >>> +++ b/src/conf/node_device_conf.c

> >>> @@ -2425,6 +2425,9 @@ virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)

> >>>  {

> >>>      virNodeDevCapsDefPtr cap = def->caps;

> >>>

> >>> +    if (def->testdriver)

> >>> +        return 0;

> >>> +

> >>>      while (cap) {

> >>>          switch (cap->data.type) {

> >>>          case VIR_NODE_DEV_CAP_SCSI_HOST:

> >>> diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h

> >>> index 685ae3034..665f766e2 100644

> >>> --- a/src/conf/node_device_conf.h

> >>> +++ b/src/conf/node_device_conf.h

> >>> @@ -316,6 +316,7 @@ struct _virNodeDeviceDef {

> >>>      char *driver;                       /* optional driver name */

> >>>      char *devnode;                      /* /dev path */

> >>>      char **devlinks;                    /* /dev links */

> >>> +    bool testdriver;                    /* if true, skip host checks */

> >>

> >> Not sure this should be in virNodeDeviceDef... I think it should be in

> >> virNodeDeviceObj. Yes, a bit more work, but I think cleaner. You'd need

> >> to create an accessor function in order to set the flag from

> >> test_driver. Then avoid calling virNodeDeviceUpdateCaps only from

> >> virNodeDeviceMatch if the flag is set.

> >>

> >> Also instead of "testdriver", how about "skipUpdateCaps" since the

> >> purpose of this is to skip calling virNodeDeviceUpdateCaps?


Or simply update_caps, but I agree with your point...

> >>

> >> At least that hides that this currently is only for the test driver.

> >> Perhaps in the future there could be some other reason to not want to

> >> update the caps for some specific definition after perhaps it's "known"

> >> or "determined" that a specific update had occurred.

> >

> > Okay, thanks for review. How about finding a way to remove UpdateCaps

> > from generic conf.c implementations instead? Seems wrong that something

> > functions needed to implement ListAllDevices touch host resources, I


Yeah, you're right, we shouldn't touch the host...

> > don't think other objects work like that

> >

>

> The call to add update for export was a fairly recent, commit id

> 'd18feadc'. Seems that Erik Skultety has the most recent knowledge.

> Hopefully he reads and chimes in...


Sorry for not responding earlier, this indeed has been on my radar since the
date of its posting, but I couldn't come up with anything better...

>

> The nodedev driver isn't like other drivers w/r/t being able to define

> or create defs on your own. Instead you rely on udev (or gasp hal) in

> order to provide "events" that would add, change, delete the device and

> get it's capabilities.

>

> It gets worse because even though an event is fired, it doesn't mean the

> update has really occurred. I know mdevs and npiv scsi_host/target's are

> afflicted by similar problems - udev says it's got an event, but some

> other layer such as systemd is still filling in the data.


Exactly, there's a kernel BZ on this and so far it looks like kernel's going to
remain shrugging their shoulders and say, it's application layer's
responsibility to figure out whether a device has been changed or not.

[...]

> I wonder if it would be better to have some sort of refresh logic that

> would do the same/similar type magic at least with respect to getting

> updates for the 4 types (SCSI_HOST, SCSI_TARGET, NET, PCI_DEV) that seem

> to be lagging or needing to get the latest information. Ironically I

> would half expect MDEV or MDEV_TYPES to be included in the need update

> list if only because of the similar issues to npiv at least w/r/t

> systemd interaction. In any case, I'm not sure there's a simple way to

> "know" your data is out of date.o


MDEV behaves like SRIOV here, i.e. whenever the host driver for PCI changes,
all the nested capabilities related to PCI are affected and as you say, we
can't reliably tell when that's happened, so we have to update them every time
there's a query for node devices.

>

> Perhaps the UpdateCaps call for Export isn't necessary, but I can see a

> reason for it - that "automagic update" functionality. Just because it

> doesn't work well for the test driver isn't perhaps a "good enough"

> reason to just remove it.

>


You need to update the caps on each API. Since you mentioned having some
refresh logic, that would be nice, although I'm not sure how that would help,
depends on design, whether it's an on demand API or some other internal logic,
because on demand doesn't really solve the problem here. Anyways, back to the
point...If you look at this patch [1], you'll see the original proposal which
was doing the update before issuing the intended API itself, I agree that this
solution wouldn't have caused the issue with the test driver we're having, but
also if you look closely at what nodeConnectUpdateAllNodeDevicesCaps was
supposed to do, it called *ObjListExport (then iterate over the whole list)
only to essentially call *ObjListExport again. Even though the time complexity
was still 3*O(n) ~= O(n), I felt like we were using unnecessary CPU cycles here
which could have been achieved as part of generating the list, since we already
apply filters on the objects to filter them out of the resulting list. And
that's what I did with my series. Now, to be fair, I was a bit hasty with my
patches too, since I put the cap update into virNodeDeviceMatch which is only
ever called from *ObjListExport, which means that nodeNumOfDevices and
nodeListDevices still didn't work properly, hence my new patch [2].

Now back to your patch. I still think that updating capabilities makes complete
sense in current form, however, not for test driver. So, at first I was thinking
about whether we could pass a different 'dummy' callback for the update from the
test driver somehow, so that the logic of 'updating the caps' would be
preserved, but I don't think it's worth it and also it might not be even
possible without putting significant effort into it. We could however go back
(sort of) to the original proposal and call the update explicitly after
obtaining the list of devices, which as I said is a bit ineffective, or (and I
prefer this) we could go your way, but here I agree with John's point to move
the flag to the object rather than the definition, since @def should serve as
a description of the object, and having such a flag within @def seems
conceptually wrong to me.

Thanks,
Erik

[1] https://www.redhat.com/archives/libvir-list/2018-January/msg00316.html
[2] https://www.redhat.com/archives/libvir-list/2018-March/msg00280.html

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patch

diff --git a/src/conf/node_device_conf.c b/src/conf/node_device_conf.c
index fd8f4e4a9..90c940f11 100644
--- a/src/conf/node_device_conf.c
+++ b/src/conf/node_device_conf.c
@@ -2425,6 +2425,9 @@  virNodeDeviceUpdateCaps(virNodeDeviceDefPtr def)
 {
     virNodeDevCapsDefPtr cap = def->caps;
 
+    if (def->testdriver)
+        return 0;
+
     while (cap) {
         switch (cap->data.type) {
         case VIR_NODE_DEV_CAP_SCSI_HOST:
diff --git a/src/conf/node_device_conf.h b/src/conf/node_device_conf.h
index 685ae3034..665f766e2 100644
--- a/src/conf/node_device_conf.h
+++ b/src/conf/node_device_conf.h
@@ -316,6 +316,7 @@  struct _virNodeDeviceDef {
     char *driver;                       /* optional driver name */
     char *devnode;                      /* /dev path */
     char **devlinks;                    /* /dev links */
+    bool testdriver;                    /* if true, skip host checks */
     virNodeDevCapsDefPtr caps;          /* optional device capabilities */
 };
 
diff --git a/src/test/test_driver.c b/src/test/test_driver.c
index 043caa976..39784c9fa 100644
--- a/src/test/test_driver.c
+++ b/src/test/test_driver.c
@@ -1165,6 +1165,7 @@  testParseNodedevs(testDriverPtr privconn,
         if (!def)
             goto error;
 
+        def->testdriver = true;
         if (!(obj = virNodeDeviceObjListAssignDef(privconn->devs, def))) {
             virNodeDeviceDefFree(def);
             goto error;
@@ -5565,6 +5566,7 @@  testNodeDeviceMockCreateVport(testDriverPtr driver,
         caps = caps->next;
     }
 
+    def->testdriver = true;
     if (!(obj = virNodeDeviceObjListAssignDef(driver->devs, def)))
         goto cleanup;
     def = NULL;