On 07/05/2012 10:51 AM, Kevin Wolf wrote:
Am 05.07.2012 16:22, schrieb Corey Bryant:
For some examples:
1. client calls 'add-fd', qemu is now tracking fd=4 with refcount
1, in
use by monitor, as member of fdset1
2. client crashes, so all tracked fds are visited; fd=4 had not yet
been
passed to 'remove-fd', so qemu decrements refcount; refcount of
fd=4 is
now 0 so qemu closes it
Doesn't the refcount belong to an fdset rather than a single fd? As
long
as the fdset has still a reference, it's possible to reach the other
fds
in the set through a reopen. For example:
1. client calls 'add-fd', qemu is now tracking fd=4 (O_RDWR) as a
member
of fdset1, in use by monitor, refcount 1
2. client calls 'add-fd', qemu is now tracing fd=5 (O_RDONLY) as a
member of fdset, in use by monitor, refcount is still 1
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 2
4. client crashes, so all tracked fdsets are visited; fdset1 gets its
refcount decreased to 1, and both member fds 4 and 5 stay open.
If you had refcounted a single fd, fd 5 would be closed now and qemu
couldn't switch to O_RDONLY any more.
If the O_RDONLY is for a future command, it would make more sense to add
that fd before that future command. Or are you passing it at step 2
because it is needed for the probe re-open issue?
Come on, requiring realistic examples isn't fair. ;-)
Heh :)
Swap steps 2 and 3 in the example, then step 3 is just the preparation
for a command that uses the new fd. The problem stays the same. Or a
live commit operation could be in flight so that qemu must be able to
switch on completion without libvirt interaction.
Good point.
I thought it would be useful to run through the examples again with each
fdset having a refcount, and each fd having an in-use flag. One
difference though is that refcount is only incremented/decremented when
qemu_open/qemu_close are called. I think this works and covers Kevin's
concerns. Actually it might be a bit simpler too.
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 crashes, so all tracked fds are visited; fd=4 had not yet been
passed to 'remove-fd', so it's in-use flag is on; in-use flag is turned
off and fd=4 is left open because it is still in use by the block device
(refcount is 1)
4. client re-establishes QMP connection, so all tracked fds are visited,
and in-use flags are turned back on; 'query-fds' lets client learn about
fd=4 still being open as part of 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 '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
There are enough reasons that an fd in an fdset exists and is not
opened, but I can't think of one why it should be dropped.
In use by whom? If it's still in use in qemu (as in "in-use flag
would
be set") and we have a refcount of zero, then that's a bug.
In use by qemu. I don't think it's a bug. I think there are
situations
where refcount gets to zero but qemu is still using the fd.
I think the refcount being non-zero _is_ what defines an fd as
being in
use by qemu (including use by the monitor). Any place you have to
close
an fd before reopening it is dangerous; the safe way is always to open
with the new permissions before closing the old permissions.
There is one case I'm aware of where we need to be careful: Before
opening an image, qemu may probe the format. In this case, the image
gets opened twice, and the first close comes before the second open.
I'm
not entirely sure how hard it would be to get rid of that behaviour.
If we can't get rid of it, we have a small window that the refcount
doesn't really cover, and if we weren't careful it would become racy.
This is why I mentioned earlier that maybe we need to defer the
refcount
decrease a bit. However, I can't see how the in-use flag would make a
difference there. If the refcount can't cover it, the in-use flag can't
either.
Yeah this is a problem. Could we introduce another flag to cover this?
Adding more refcounts or flags is not a problem, but it doesn't solve it
either. The hard question is when to set that flag.
I believe it may be easier to just change the block layer so that it
opens files only once during bdrv_open().