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

On 05/23/2017 04:13 AM, Bjoern Walk wrote: "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