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

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

On 07/03/2012 01:03 PM, Eric Blake wrote:
> On 07/03/2012 10:25 AM, Corey Bryant wrote:
>
>>> I thought qemu would rather return the number of the fdset (which it
>>> also assigns if none it passed, i.e. for fdset creation). Does libvirt
>>> need the number of an individual fd?
>>>
>>> If libvirt prefers to assign fdset numbers itself, I'm not against it,
>>> it's just something that wasn't clear to me yet.
>>>
>>
>> That's fine.  QEMU can return the fdset number or a string
>> (/dev/fdset/1) if none is specified.  And an fdset will need to be
>> specified if adding to an existing set.
>>
>> I think libvirt will need the fd returned by add-fd so that it can
>> evaluate fds returned by query-fd.  It's also useful for remove-fd.
>
> Correct - since we will be adding a remove-fd, then that command needs
> to know both the fdset name and the individual fd within the set to be
> removed.
>

Ok

>>
>>>> 2. drive_add file=/dev/fdset/1 -> qemu_open uses the first fd from the
>>>> set that has access flags matching the qemu_open action flags.
>>>> qemu_open increments refcount for this fd.
>>>> 3. add-fd /dev/fdset/1 FDSET={M} -> qemu adds fd to set named
>>>> "/dev/fdset/1" - command returns qemu fd to caller (e.g fd=5).  libvirt
>>>> in-use flag turned on for fd.
>>>> 3. block-commit -> qemu_open reopens "/dev/fdset/1" by using the first
>>>> fd from the set that has access flags matching the qemu_open action
>>>> flags.  qemu_open increments refcount for this fd.
>>>> 4. remove-fd /dev/fdset/1 5 -> caller requests fd==5 be removed from the
>>>> set.  turns libvirt in-use flag off marking the fd ready to be closed
>>>> when qemu is done with it.
>>>
>>> If we decided to not return the individual fd numbers to libvirt, file
>>> descriptors would be uniquely identified by an fdset/flags pair here.
>>>
>>
>> Are you saying we'd pass the fdset name and flags parameters on
>> remove-fd to somehow identify the fds to remove?
>
> Passing the flag parameters is not trivial, as that would mean the QMP
> code would have to define constants mapping to all of the O_* flags that
> qemu_open supports.  It's easier to support closing by fd number.
>

I understand what you were saying now, although I guess it's not 
applicable at this point.  I'll plan on returning the fd from add-fd and 
passing the fd on the remove-fd command.

>>
>>>> 5. qemu_close decrements refcount for fd, and closes fd when refcount is
>>>> zero and libvirt in use flag is off.
>>>
>>> The monitor could just hold another reference, then we save the
>>> additional flag. But that's a qemu implementation detail.
>>>
>>
>> I'm not sure I understand what you mean.
>
> pass-fd (or add-fd, whatever name we give it) adds an fd to an fdset,
> with initial use count of 1 (the use is the monitor).  qemu_open()
> increments the use count.  A new qemu_close() wrapper would decrement
> the use count.  And both calling 'remove-fd', or closing the QMP monitor
> of an fd that has not yet been passed through 'remove-fd', serves as a
> way to decrement the use count.  You'd still have to track whether the
> monitor is using an fd (to avoid over-decrementing on QMP monitor
> close), but by having the monitor's use also tracked under the refcount,
> then refcount reaching 0 is sufficient to auto-close an fd.  I think
> that also means that re-establishing the client QMP connection would
> increment  For some examples:

Yes, I think adding a +1 to the refcount for the monitor makes sense.

I'm a bit unsure how to increment the refcount when a monitor reconnects 
though.  Maybe it is as simple as adding a +1 to each fd's refcount when 
the next QMP monitor connects.

>
> 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

Just a note that the fd above also hasn't yet been referenced by a 
drive-add/device-add, so it will be closed in step 2.

>
> 1. client calls 'add-fd', qemu is now tracking fd=4 with refcount 1, in
> use by monitor, as member of fdset1
> 2. client calls 'device-add' with /dev/fdset/1 as the backing filename,
> so qemu_open() increments the refcount to 2
> 3. client crashes, so all tracked fds are visited; fd=4 had not yet been
> passed to 'remove-fd', so qemu decrements refcount to 1, but leaves fd=4
> open because it is still in use by the block device
> 4. client re-establishes QMP connection, and 'query-fds' lets client
> learn about fd=4 still being open as part of fdset1, but also informs
> client that fd is not in use by the monitor

And in step 4 the QMP connection will increment the refcount +1 for all 
fds that persisted through the QMP disconnect. (?)

>
> 1. client calls 'add-fd', qemu is now tracking fd=4 with refcount 1, in
> use by monitor, as member of fdset1
> 2. client calls 'device-add' with /dev/fdset/1 as the backing filename,
> so qemu_open() increments the refcount to 2
> 3. client calls 'remove-fd fdset=1 fd=4', so qemu marks fd=4 as no
> longer in use by the monitor, refcount decremented to 1 but still left
> open because it is in use by the block device
> 4. client crashes, so all tracked fds are visited; but fd=4 is already
> marked as not in use by the monitor, so its refcount is unchanged
>
> 1. client calls 'add-fd', qemu is now tracking fd=4 with refcount 1, in
> use by monitor, as member of fdset1
> 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 marks fd=4 as no longer in use by the
> monitor, refcount is now decremented to 0 and fd=4 is closed
>
> I think that covers the idea; you need a bool in_use for tracking
> monitor state (the monitor is in use until either a remove-fd or a
> monitor connection closes), as well as a ref-count.
>

Yes, it all makes sense to me.  Thanks for the scenarios.

>>> We also need a query-fdsets command that lists all fdsets that exist. If
>>> we add information about single fds to the return value of it, we
>>> probably don't need a separate query-fd that operates on a single fdset.
>>>
>>
>> Yes, good point.  And maybe we don't need 2 commands.  query-fdsets
>> could return all the sets and all the fds that are in those sets.
>
> Yes, I think a single query command is good enough here, something like:
>
> { "execute":"query-fdsets" } =>
> { "return" : { "sets": [
>     { "name": "fdset1",
>       "fds": [ { "fd": 4, "monitor": true, "refcount": 1 } ] },
>     { "name": "fdset2",
>       "fds": [ { "fd": 5, "monitor": false, "refcount": 1 },
>                { "fd": 6, "monitor": true, "refcount": 2 } ] } ] } }
>
>

Ok, thanks!

>>> 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.
>

Maybe Kevin wants to weigh in on this.  Perhaps it's an issue that can 
be separated from my patch series.

-- 
Regards,
Corey