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