[libvirt] [PATCH 04/13] nodedev: Move node_device_linux_sysfs from node_driver to conf

Wed May 24 13:45:09 UTC 2017

On 05/23/2017 04:13 AM, Bjoern Walk wrote:
> John Ferlan <jferlan at redhat.com> [2017-05-19, 09:08AM -0400]:
>> Move the whole file from src/node_device into src/conf and rename the
>> API's to have the "virNodeDevice" prefix.
>>
>> Signed-off-by: John Ferlan <jferlan at redhat.com>
>> ---
>> src/Makefile.am                                    |  8 ++++----
>> .../node_device_linux_sysfs.c                      | 24
>> ++++++++++------------
>> .../node_device_linux_sysfs.h                      |  9 +++++---
>> src/libvirt_private.syms                           |  5 +++++
>> src/node_device/node_device_driver.c               |  8 ++++----
>> src/node_device/node_device_hal.c                  |  4 ++--
>> src/node_device/node_device_udev.c                 |  4 ++--
>> 7 files changed, 34 insertions(+), 28 deletions(-)
>> rename src/{node_device => conf}/node_device_linux_sysfs.c (89%)
>> rename src/{node_device => conf}/node_device_linux_sysfs.h (82%)
>>
> 
> What's the reasoning for this move? It somehow doesn't feel right and it
> will make backports harder.

"Eventually" code that is currently in node_device_driver to peruse the
node device object list is going to move to virnodedevobj.c and there
was some "boundary" thing about calling back into the driver that I
don't quite recall the exact compile/link error which caused me to move
the code. Although the commit date is relatively recent - it's more or
less a copy of work done in Dec 2016.

One could also make the argument that there's nothing "driver specific"
in the code, so why should it have ever been put in the src/node_device
directory?  My other option would be to essentially move the guts of the
code into virnodedevobj and call it from src/node_device/...  IDC which
way this happens, but I just found this easier. Using gitk I've never
had issues in the past following history of the code when a module moves
from one directory to another.

FWIW: My first instinct was move the code to src/util like other code
that has linux (or arch specific) pieces, but I couldn't do that because
node_device_linux_sysfs.h references "node_device_conf.h". Inclusion of
conf/*.h has been disallowed from src/util.

So to answer the other point... I do not believe that making backports
harder should be a reason to limit changes, but I also understand that's
a touchy subject to many. Still when I first started working on libvirt
there was quite a bit of code refactoring and movement - it was at times
difficult to follow, but it subsided fairly quickly and I learned how to
use gitk to follow changes. As an aside, for any of this common object
model work if I have come across something that I consider a "bug" I've
tried to "front load" that to the beginning of the patch series so that
it is easier to cherry-pick to some backport. In the long run any of
these changes is only painful when it affects the code you care most
about. I would submit that that nodedev code has long been "overlooked"
for optimizations and change to bring it more in line with how libvirt
is currently structured. I've even wondered if HAL is even relevant any
more, but I don't want to go down that path!

Couple of other "datapoints" to consider.

  1. The impetus for all the changes I've posted in various drivers and
conf/vir*obj.c module is to use a common object model to handle the
functions that every driver has essentially cut-n-paste'd (create, add,
remove, FindBySomeKey, NumOf, return ListOfSomeKey, and return
ListOfExternalReference). Over time each has varied slightly or fixed
bugs. Late last year there was a new driver proposed from Virtuozzo
which essentially copied everything from the storage pool into it's own
model. That got me to looking at the code and thinking there's no way
the model should be to essentially copy everything and just change the
names. In the long run all the drivers have their own XML/data
definition, but there's no way they shouldn't be able to share object
handling. It's taking a while to get there, but I think when it's done
it'll be easier to maintain.

 2. The current object handling model is scattered. Some of the drivers
are more active (domain, network, storage) and each could have some fix
applied to one that isn't applied to another that is probably a bug in
the other. If you have common code handling the objects for each driver,
then fixing one fixes all. Reading the code and trying to compare how
some other driver did things to fit into another driver to fix a similar
issue seen can be painful.  Been there while working through making the
secret driver use hash tables (my model varied slightly between domain
and network depending on which I thought did something better).

 3. Most of the drivers use forward linked lists to manage their objects
(nodedev, interface, nwfilter, storagepool, storagevol), while others
use hash tables (domain, domainsnapshot, network, secret). Those forward
linked list drivers roll their own code w/r/t object mutexes which have
long been provided by virObjectLockable. These should have been
converted ages ago, but no one's taken the time to do so.

  3a. In particular, nodedev uses a forward linked list.  Ironically,
for nodedev "performance" this usually means the bulk of object lookups
have to traverse potentially long lists to find what's being searched
most frequently. Compare that to a hash table lookup. On my work laptop
there are 140 nodedev objs. Of those I usually want to reference the
ones added last - e.g iSCSI luns for "pseudo" block devices. Those
block_*, scsi_host*, and scsi_generic* are always going to be at the end
just by the nature of the udev discovery algorithm. I would assume this
is much more so true for those "larger" environments as well, but I
don't have empirical evidence.

  If you consider that a volume for a storage pool could use some
nodedev list in order to essentially "find" the storage it wants. In
order to find the pool, follow a linked list of pools to find your
specific pool. Then follow a linked list to find the specific volume
within the pool. Then for some volumes this also means following the
nodedev list in order to find system representation for it.  If you
start with a pool w/ 0 volumes, then as you add 10, 20, 100, 200, 1000,
etc. volumes I would hope you could see the scaling problem that a
forward linked list for each of these drivers would present.  Now change
all those to use a hash table lookup and consider your performance.

I don't disagree this is painful, but it could also be short lived pain
depending on reviews. For a reference to the object and even more
history see:

https://www.redhat.com/archives/libvir-list/2017-April/msg00321.html

In my branches the order and quantity of those patches has changed. I
plan to post an update shortly, but wanted to get through the bulk of
the changes I originally posted in February before reposting (plus I
needed to add a few more comments).

John