On Thursday 31 May 2012 at 10:44, Frido Roose wrote:

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

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

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?