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().
Kevin
--
Regards,
Corey