Re: [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd

[Corey Bryant <coreyb@linux.vnet.ibm.com>]

On 07/06/2012 05:11 AM, Kevin Wolf wrote:
> Am 05.07.2012 19:00, schrieb Eric Blake:
>> On 07/05/2012 10:35 AM, Corey Bryant wrote:
>>> 1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
>>> refcount of 0; fd=4's in-use flag is turned on
>>> 2. client calls 'device-add' with /dev/fdset/1 as the backing filename,
>>> so qemu_open() increments the refcount of fdset1 to 1
>>> 3. client calls 'remove-fd fdset=1 fd=4', so qemu marks fd=4 as no
>>> longer in-use by the monitor, and is left open because it is in use by
>>> the block device (refcount is 1)
>>> 4. client crashes, so all tracked fds are visited; fd=4 is not in-use
>>> but refcount is 1 so it is not closed
>> 5. client re-establishes QMP connection, so all tracked fds are visited.
>>   What happens to the fd=4 in-use flag?
>>
>> ...but what are the semantics here?
>>
>> If we _always_ turn the in-use flag back on, then that says that even
>> though libvirt successfully asked to close the fd, the reconnection
>> means that libvirt once again has to ask to close things.
>>
>> If we _never_ turn the in-use flag back on, then we've broken the first
>> case above where we want an in-use fd to come back into use after a crash.
>>
>> Maybe that argues for two flags per fd: an in-use flag (there is
>> currently a monitor connection that can manipulate the fd, either
>> because it passed in the fd or because it reconnected) and a removed
>> flag (a monitor called remove-fd, and no longer wants to know about the
>> fd, even if it crashes and reconnects).
>
> I was in fact just going to suggest a removed flag as well, however
> combined with including the monitor connections in the refcount instead
> of an additional flag. This would also allow to have (the currently
> mostly hypothetical case of) multiple QMP monitors without interesting
> things happening.
>
> Maybe I'm missing some point that the inuse flag would allow and
> including monitors in the refcount doesn't. Is there one?
>
> Kevin
>

I think we need the granularity of having an in-use flag per fd.  Here's 
an example of why:

1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 1; fd=4's remove flag is initialized to off
2. client calls 'device-add' with /dev/fdset/1 as the backing filename;
qemu_open() increments the refcount of fdset1 to 2
3. client crashes, so all fdsets are visited; fd=4 had not yet been
passed to 'remove-fd', so it's remove flag is off; refcount for fdset1 
is decremented to 1; fd=4 is left open because it is still in use by the 
block device (refcount is 1)
4. client re-establishes QMP connection, refcount for fdset1 is 
incremented to 2; 'query-fds' lets client learn about fd=4 still being 
open as part of fdset1
5. qemu_close is called for fd=4; refcount for fdset1 is decremented to 
1; fd=4 remains open as part of fdset1
6. QMP disconnects; refcount for fdset is decremented to zero;  fd=4 is 
closed and fdset1 is freed

1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 1 (because of monitor reference); fd=4's remove flag is 
initialized to off
2. client calls 'device-add' with /dev/fdset/1 as the backing filename,
but the command fails for some other reason, so the refcount is still 1
at the end of the command (although it may have been temporarily
incremented then decremented during the command)
3. client calls 'remove-fd fdset=1 fd=4' to deal with the failure (or
QMP connection is closed), so qemu turns on the remove flag for fd=4; 
refcount is 0 so all fds in fdset1 are closed


I think we need the granularity of having an in-use flag per fd.  If we 
track monitor in-use in the reference count, then we are tracking it for 
the fdset and I think this could cause leaks.

In the following example, we have a refcount for the fdset, an in-use 
flag per fd, and a remove flag per fd.  We're only 
incrementing/decrementing refcount in qemu_open/qemu_close.





In the following example, we have a refcount for the fdset, and a remove 
flag per fd.  We're incrementing refcount when the fdset is first 
created, when QMP re-connects, and in qemu_open.  We're decrementing 
refcount when QMP disconnects, and in qemu_close.

1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 1; fd=4's remove flag is initialized to off
2. client crashes, so all tracked fdsets are visited; fd=4 has
not yet been passed to 'remove-fd', so its remove flag is off; in-use
flags are turned off and both fds are left open because the set is still
in use by the block device (refcount is 1)








1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 1; fd=4's remove flag is initialized to off
2. client calls 'device-add' with /dev/fdset/1 as the backing filename;
qemu_open() increments the refcount of fdset1 to 2
3. client crashes, so all fdsets are visited; fd=4 had not yet been
passed to 'remove-fd', so it's remove flag is off; refcount for fdset1 
is decremented to 1; fd=4 is left open because it is still in use by the 
block device (refcount is 1)
4. client re-establishes QMP connection, refcount for fdset1 is 
incremented to 2; 'query-fds' lets client learn about fd=4 still being 
open as part of fdset1
5. qemu_close is called for fd=4; refcount for fdset1 is decremented to 
1; fd=4 remains open as part of fdset1
6. QMP disconnects; refcount for fdset is decremented to zero;  fd=4 is 
closed and fdset1 is freed

1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 0; fd=4's in-use flag is turned on
2. client calls 'device-add' with /dev/fdset/1 as the backing filename,
so qemu_open() increments the refcount of fdset1 to 1
3. client calls 'remove-fd fdset=1 fd=4', so qemu marks fd=4 as no
longer in-use by the monitor, and is left open because it is in use by
the block device (refcount is 1)
4. client crashes, so all tracked fds are visited; fd=4 is not in-use
but refcount is 1 so it is not closed

1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 0; fd=4's in-use flag is turned on
2. client calls 'device-add' with /dev/fdset/1 as the backing filename,
but the command fails for some other reason, so the refcount is still 0
at the end of the command (although it may have been temporarily
incremented then decremented during the command)
3. client calls 'remove-fd fdset=1 fd=4' to deal with the failure (or
QMP connection is closed), so qemu marks fd=4 as no longer in-use by the
monitor; refcount is 0 so all fds in fdset1 are closed

1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 0; fd=4's in-use flag is turned on
2. client calls 'add-fd', qemu is now tracking fd=5 in fdset1 with
refcount still 0; fd=5's in-use flag is turned on
3. client crashes, so all tracked fds are visited; fd=4 and fd=5 had not
yet been passed to 'remove-fd', so their in-use flags are on; in-use
flags are turned off; refcount of fdset1 is 0 so qemu closes all fds in
fdset1

1. client calls 'add-fd', qemu is now tracking fd=4 in fdset1 with
refcount of 0; fd=4's in-use flag is turned on
2. client calls 'add-fd', qemu is now tracking fd=5 in fdset1 with
refcount still 0; fd=5's in-use flag is turned on
3. client calls 'remove-fd fdset=1 fd=4', so qemu marks fd=4 as no
longer in-use by the monitor, and fd=4 is closed since the refcount is
0; fd=5 remains open

1. client calls 'add-fd', qemu is now tracking fd=4 (O_RDWR) in fdset1
with refcount of 0; fd=4's in-use flag is turned on
2. client calls 'add-fd', qemu is now tracking fd=5 (O_RDONLY) in fdset1
with refcount still 0; fd=5's in-use flag is turned on
3. client calls 'device-add' with /dev/fdset/1 as the backing filename
and r/w, so qemu_open() dup()s fd 4 and increments the refcount to 1
4. client calls 'remove-fd fdset=1 fd=4', so qemu marks fd=4 as no
longer in-use by the monitor, but fd=4 remains open because refcount of
fdset1 is 1
5. client calls 'remove-fd fdset=1 fd=5', so qemu marks fd=5 as no
longer in-use by the monitor, and fd=5 remains open because refcount of
fdset1 is 1
6. qemu_close(fd=4) is called, refcount of fdset1 is decremented; both
fds are closed since refcount is 0 and their in-use flags are off

1. client calls 'add-fd', qemu is now tracking fd=4 (O_RDWR) in fdset1
with refcount of 0; fd=4's in-use flag is turned on
2. client calls 'add-fd', qemu is now tracking fd=5 (O_RDONLY) in fdset1
with refcount still 0; fd=5's in-use flag is turned on
3. client calls 'device-add' with /dev/fdset/1 as the backing filename
and r/w, so qemu_open() dup()s fd 4 and increments the refcount to 1
4. client crashes, so all tracked fds are visited; fd=4 and fd=5 have
not yet been passed to 'remove-fd', so their in-use flags are on; in-use
flags are turned off and both fds are left open because the set is still
in use by the block device (refcount is 1)
5. qemu_close(fd=4) is called, refcount of fdset1 is decremented; both
fds are closed since refcount is 0 and their in-use flags are off


-- 
Regards,
Corey