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