Re: [libvirt] [PATCH v3 14/28] lock_daemon_dispatch: Check for ownerPid rather than ownerId
On 09/03/2018 11:13 AM, Michal Privoznik wrote:
On 08/30/2018 11:47 PM, John Ferlan wrote:
>
>
> On 08/27/2018 04:08 AM, Michal Privoznik wrote:
>> At the beginning of each dispatch function we check if owner
>> attributes were registered (these consist of ID, UUID, PID and
>> name). The check then consists of checking if ID is not zero.
>> This is not going to work with
>> VIR_LOCK_MANAGER_OBJECT_TYPE_DAEMON which doesn't set ID. Switch
>> to setting PID which is available for both cases.
>>
>> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com>
>> ---
>> src/locking/lock_daemon_dispatch.c | 14 +++++++-------
>> 1 file changed, 7 insertions(+), 7 deletions(-)
>>
>
> BTW: My idea of setting id == -1 for deamon still works without any
> change required in/for this patch.
>
> So what would be concerning about using ownerPid would be that over the
> lifetime of the host the @pid can recycle; whereas, during the lifetime
> of the daemon don't we guarantee that the @id will be ever increasing?
> But right now I'm too lazy to go look and see if getting the next id is
> through libvirtd or virtlockd.
>
> Not against this, but I need to get feedback from earlier patches and of
> course your thoughts on the @id vs. @pid rotation. Plus I need see how
> this plays out in future patches.
The fact that @id always increases plays no role here and also is not
true. It's only qemu driver that has increasing IDs. For instance, libxl
driver which also uses virtlockd generates 'random' domain IDs.
I saw your query on #virt ;-)
Pid rotation is no problem here. What my patches currently implement is:
libvirtd registers itself as owner of the lock (with its PID). It has to
keep the connection to virtlockd open since the first lock() to the very
last unlock(), otherwise virtlockd sees EOF on the connection, and
starts releasing locks associated with the PID (=libvirtd) killing the
PID too [1]. So there is no way another process can emerge with the same
PID and magically keep the locks or something. At the moment libvirtd
dies the connection is closed and virlockd releases the locks. There's
no window for PID wrap.
Well, we know our consumer libvirtd is well behaved. Of course
recalling offhand while reviewing whether the magic of properly handling
what to do on EOF or closure is a challenge.
Still based on the fact that @id are random w/ libxl means that while
unlikely it is possible for reuse too and at least with PID's we have
assurances that the lock manager is handling the "removal" of locks when
the EOF is seen, so going with PID's is fine.
Maybe a blurb that indicates what lock manager magic allows us to
consider why usage of PID's even with the knowledge they can turn over
makes it more desirable to use.
John
And since for OBJECT_TYPE_DAEMON there is no 'id' (becuase it doesn't
make sense to have one) we have to meet at common ground => PID. Every
process has one (except kernel jobs, but we do not care about those).
We might as well check for owner name.
Michal
1 - this is correct thing to do. We want virlockd to release all the
resources owned by PID when it sees EOF because it acts like regular
file locks. When a process is dying all file locks it has are also
released. More on that in later patches.