Re: [libvirt] Bug report 826704 - sanlock releases all resources on virsh detach-disk

On Thursday 31 May 2012 at 10:44, Frido Roose wrote: Hello, I logged a bug about using virsh detach-disk cleaning up all sanlock resources for the domain instead of only the device in question. After a quick look into the code, I think a new method similar to virLockManagerSanlockAddResource is needed in case of detaching a disk from the domain, like e.g. virLockManagerSanlockDelResource (…). Now it looks like virLockManagerSanlockRelease is called, which releases all resources: if ((rv = sanlock_release(-1, priv->vm_pid, SANLK_REL_ALL, 0, NULL)) < 0) { virsh detach-disk should then call virLockManagerSanlockDelResource for the given resource imo. Any thoughts about this or why it is implemented like this? With the help of some debugging output and the source code, new questions arise: Reproducing the problem =================== 10:10:47 pan ~ # virsh domblklist vmor01 Target Source ------------------------------------------------ vda /var/lib/libvirt/images/vmor01.img vdb /dev/disk/by-id/wwn-0x600a0b80002695880000cc4c4f865710 10:10:51 pan ~ # sanlock client status daemon b7baf6f0-2d9b-4b6f-be1e-2169284c68a0.pan p -1 listener p 9846 p -1 status s __LIBVIRT__DISKS__:14:/var/lib/libvirt/sanlock/__LIBVIRT__DISKS__:0 r __LIBVIRT__DISKS__:d540f8af975019b0d41c4c65e4955072:/var/lib/libvirt/sanlock/d540f8af975019b0d41c4c65e4955072:0:11 p 9846 r __LIBVIRT__DISKS__:571a282fc5e8cbb42167ce9ec1d41a95:/var/lib/libvirt/sanlock/571a282fc5e8cbb42167ce9ec1d41a95:0:10 p 9846 10:10:53 pan ~ # virsh detach-disk vmor01 vdb Disk detached successfully 10:11:39 pan ~ # sanlock client status daemon b7baf6f0-2d9b-4b6f-be1e-2169284c68a0.pan p -1 listener p 9846 p -1 status s __LIBVIRT__DISKS__:14:/var/lib/libvirt/sanlock/__LIBVIRT__DISKS__:0 DEBUG output ============ 2012-06-01 08:11:39.198+0000: 9718: debug : virDomainLockManagerNew:123 : plugin=0x7fa5240cb4f0 dom=0x7fa52419d460 withResources=0 2012-06-01 08:11:39.198+0000: 9718: debug : virLockManagerNew:291 : plugin=0x7fa5240cb4f0 type=0 nparams=4 params=0x7fa533cdf8d0 flags=0 2012-06-01 08:11:39.198+0000: 9718: debug : virLockManagerLogParams:98 : key=uuid type=uuid value=c0ee1dfa-4056-f3f9-9f8c-f10e974b59f0 2012-06-01 08:11:39.198+0000: 9718: debug : virLockManagerLogParams:94 : key=name type=string value=vmor01 2012-06-01 08:11:39.198+0000: 9718: debug : virLockManagerLogParams:82 : key=id type=uint value=1 2012-06-01 08:11:39.198+0000: 9718: debug : virLockManagerLogParams:82 : key=pid type=uint value=9846 2012-06-01 08:11:39.198+0000: 9718: debug : virDomainLockManagerAddDisk:86 : Add disk /dev/disk/by-id/wwn-0x600a0b80002695880000cc4c4f865710 2012-06-01 08:11:39.198+0000: 9718: debug : virLockManagerAddResource:320 : lock=0x7fa524198360 type=0 name=/dev/disk/by-id/wwn-0x600a0b80002695880000cc4c4f865710 nparams=0 params=(nil) flags=0 2012-06-01 08:11:39.198+0000: 9718: debug : virLockManagerRelease:352 : lock=0x7fa524198360 state=(nil) flags=0 2012-06-01 08:11:39.214+0000: 9718: debug : virLockManagerFree:374 : lock=0x7fa524198360 SOURCE ======== qemuDomainDetachDiskDevice or qemuDomainDetachPciDiskDevice calls virDomainLockDiskDetach (src/locking/domain_lock.c): int virDomainLockDiskDetach(virLockManagerPluginPtr plugin, virDomainObjPtr dom, virDomainDiskDefPtr disk) { virLockManagerPtr lock; int ret = -1; if (!(lock = virDomainLockManagerNew(plugin, dom, false))) return -1; if (virDomainLockManagerAddDisk(lock, disk) < 0) goto cleanup; if (virLockManagerRelease(lock, NULL, 0) < 0) goto cleanup; ret = 0; cleanup: virLockManagerFree(lock); return ret; } I wondered why virDomainLockManagerAddDisk is called when detaching a disk device. It even looks like this succeeds and then virLockManagerRelease is called, which cleans up all sanlock resources for the domain. I suspect a function like virDomainLockManagerRemoveDisk(lock, disk) should be called at this point that releases the specific disk, which does not yet exist. Only virLockDriverAddResource exists in the _virLockDriver struct. I have the feeling that this sanlock is not completely implemented yet? Or am I missing something big? Best regards, Frido Roose