Re: [libvirt] [PATCH 1/3] Add a plugin dlm for lock manager

On 02/08/2018 03:30 AM, Lin Fu wrote: No - just trying to be thorough with respect to expectations that you're probably not aware of as someone who doesn't regularly follow or contribute to libvirt. If it came off as oh this is crap, then sorry - it wasn't intended that way. I'd rather have someone be explicit when they don't like something than make me guess what I did wrong or that wasn't following some project guidelines. Be careful how you snip - loss of context of placement... This comment was pointing out you didn't update the docs. I'd choose dlm over other options too, but that's because I have a pretty good idea of how it works and it's features. The on/off nature of pthread/mutex locks has always felt like a step backwards for sharing resources from what I cut my teeth on with the OpenVMS Lock Manager. Still you need to "sell" why using dlm is better than the existing sanlock or lockd. My "vision" of dlm is that resource you're adding that's available clusterwide in some manner should be able to have something that each member of the cluster would name the resource in the same manner. What I don't have is the mental picture of how say /dev/sdm on one host "appears" on another host. If it was /dev/sdm, then it'd be really simple to use that as a resource name and know it's the same resource between hosts. When I see the device name being hashed and added as a resource, then I wonder how different nodes in the cluster will know it's the same thing. Although I do recall some code used the Resource Name field rather judiciously to do some fairly interesting things. When you see system space (e.g. kernel) address in a resource name, you know something entertaining is occurring. So this series was really more of an RFC than a V1... It's ok, but indicating RFC sets the expectations of the reviewer as opposed to posting a V1. Following posting guidelines of running make check syntax-check would have avoided that. There are some that won't even *look* at code that fails those basic tests. Seems like Tumbleweed needs to have a bug report filed in order to get the Version properly filled in. If you live with UNKNOWN, then the problem is how do you really know if the version someone has installed will have what you want without peeking into the library and looking for specific routines. I think for some reason we do that with some of the crypto m4 files, but then again I try to avoid these files. And if /tmp is very small by some admin's choice (done for various reasons), then what happens to your code? There are mechanisms to store in XML data that needs to be saved between libvirtd restarts and even host reboots. It's really not clear where this fits. Perhaps a /var/run/libvirt type file that would be cleaned up differently. You could consider XML or JSON format... It's more of a I'm used to seeing unsigned int rather than uint32_t which is more limiting. Lots of recent examples using virHashCreate, virHashAddEntry, virHashRemoveEntry, etc. A hash table can/will resize as necessary. Assuming usage is a dangerous thing. You're not just talking domains, you're factoring in shared storage between domains and leases into your resource counts. Recent examples have been assuming no one would ever have more than (say) 200 disks defined for a domain for which we want to obtain domain statistics. Well it happened and we chased a way to handle that given the limitation of size of RPC messages. More recently some block device with more than 200 backing stores generated a very large pile of data where there was lots of duplication. Hmm. you're using virtlockd and creating lock_driver_dlm, but then virtlockd isn't running? I really didn't dig too deep and think that much about how this integrates at all. Perhaps more time spent understanding dlm interactions will help. Just copying an existing implementation without understanding what the interactions are may not fit into the model you're trying to create. I honestly don't have the cycles to learn all that dlm has to offer right now. But I do think you do need to spend more time understanding how libvirt works today and how dlm will make things better and then how exactly to accomplish that in an understandable manner. My biggest concern is someone would add a feature like this and then not continue working with/on libvirt. The dlm concepts are a departure from existing locking models. If you want to get known in the libvirt community then pick some of the simple tasks from the bite sized tasks page: https://wiki.libvirt.org/page/BiteSizedTasks that still need addressing... Again - a bit of context... This was a dlm_ls_pthread_init call made in virLockManagerDlmSetup. So something that's called once creates a thread to do what? I didn't research it, but it's something you have to know and understand so that if someone asks you can have the answer. An lksb is a "lock status block", but that's on a lock by lock basis. IIRC, those are written when lock conversions occur. If you used the "dlm_lock" API, then used the completion routines, you can then check the lksb for the status of the attempted operation. I'm very rusty in my recollection and my description is more my understanding of it. I'm still wondering why a hash is being used. That's an implementation detail which I haven't yet come to grips with given what I know from my history with OpenVMS (think about that /dev/sdm example above). In OpenVMS "disks" that were resources in a cluster shared with some SAN within the cluster connectivity fabric. Each member of the cluster had an ID (I think you have that with the CPG above) and shared resources would be named with that ID - it's been over 15 years, so too long to remember the details. Still point is, for a dlm environment to be effective device naming between cluster members is going to be important. Nothing's been accepted you'd just remove this from future patches. You need to think in terms of what libvirt consumers could use or need and perhaps not specifically something that dlm provides. Don't forget to update your local libvirt.git repo in order to add the pertinent information so that the next series you post will have it. Sorry if there's an incomplete thoughts above - I've been distracted today and this evening - it's dinner time now. John