10:05 a.m.
On 07/09/2012 10:05 AM, Luiz Capitulino wrote:
On Thu, 05 Jul 2012 11:06:56 -0400
Corey Bryant <coreyb(a)linux.vnet.ibm.com> wrote:
>
>
> On 07/04/2012 04:09 AM, Kevin Wolf wrote:
>> Am 03.07.2012 20:21, schrieb Corey Bryant:
>>> On 07/03/2012 02:00 PM, Eric Blake wrote:
>>>> On 07/03/2012 11:46 AM, Corey Bryant wrote:
>>>>
>>>>>
>>>>> 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.
>>>>
>>>> Or maybe delay a +1 until after a 'query-fds' - it is not until
the
>>>> monitor has reconnected and learned what fds it should be aware of that
>>>> incrementing the refcount again makes sense. But that would mean making
>>>> 'query-fds' track whether this is the first call since the
monitor
>>>> reconnected, as it shouldn't normally increase refcounts.
>>>
>>> This doesn't sound ideal.
>>
>> Yes, it's less than ideal.
>>
>>>> The other alternative is that the monitor never re-increments a
>>>> refcount. Once a monitor disconnects, that fd is lost to the monitor,
>>>> and a reconnected monitor must pass in a new fd to be re-associated with
>>>> the fdset. In other words, the monitor's use of an fd is a one-way
>>>> operation, starting life in use but ending at the first disconnect or
>>>> remove-fd.
>>>
>>> I would vote for this 2nd alternative. As long as we're not introducing
>>> an fd leak. And I don't think we are if we decrement the refcount on
>>> remove-fd or on QMP disconnect.
>>
>> In fact, I believe this one is even worse. I can already see hacks like
>> adding a dummy FD with invalid flags and removing it again just to
>> regain control over the fdset...
>>
>> You earlier suggestion made a lot of sense to me: Whenever a new QMP
>> monitor is connected, increase the refcount. That is, as long as any
>> monitor is there, don't drop any fdsets unless explicitly requested via QMP.
>
> Ok. So refcount would be incremented (for the fd or fdset, whatever we
> decide on) when QMP reconnects. I'm assuming we wouldn't wait until
> after a query-fds call.
I'm not sure this is a good idea because we will leak fds if the client forgets
about the fds when re-connecting (ie. it was restarted) or if a different
client connects to QMP.
If we really want to do that, I think that the right way of doing this is to
add a command for clients to re-again ownership of the fds on reconnection.
But to be honest, I don't fully understand why this is needed.
I'm not sure this is an issue with current design. I know things have
changed a bit as the email threads evolved, so I'll paste the current
design that I am working from. Please let me know if you still see any
issues.
FD passing:
-----------
New monitor commands enable adding/removing an fd to/from a set. New
monitor command query-fdsets enables querying of current monitor fdsets.
The set of fds should all refer to the same file, with each fd having
different access flags (ie. O_RDWR, O_RDONLY). qemu_open can then dup
the fd that has the matching access mode flags.
Design points:
--------------
1. add-fd
-> fd is passed via SCM rights and qemu adds fd to first unused fdset
(e.g. /dev/fdset/1)
-> add-fd monitor function initializes the monitor inuse flag for the
fdset to true
-> add-fd monitor function initializes the remove flag for the fd to false
-> add-fd returns fdset number and received fd number (e.g fd=3) to caller
2. drive_add file=/dev/fdset/1
-> qemu_open uses the first fd in fdset1 that has access flags matching
the qemu_open action flags and has remove flag set to false
-> qemu_open increments refcount for the fdset
-> Need to make sure that if a command like 'device-add' fails that
refcount is not incremented
3. add-fd fdset=1
-> fd is passed via SCM rights
-> add-fd monitor function adds the received fd to the specified fdset
(or fails if fdset doesn't exist)
-> add-fd monitor function initializes the remove flag for the fd to false
-> add-fd returns fdset number and received fd number (e.g fd=4) to caller
4. block-commit
-> qemu_open performs "reopen" by using the first fd from the fdset that
has access flags matching the qemu_open action flags and has remove flag
set to false
-> qemu_open increments refcount for the fdset
-> Need to make sure that if a command like 'block-commit' fails that
refcount is not incremented
5. remove-fd fdset=1 fd=4
-> remove-fd monitor function fails if fdset doesn't exist
-> remove-fd monitor function turns on remove flag for fd=4
6. qemu_close (need to replace all close calls in block layer with
qemu_close)
-> qemu_close decrements refcount for fdset
-> qemu_close closes all fds that have (refcount == 0 && (!inuse || remove))
-> qemu_close frees the fdset if no fds remain in it
7. disconnecting the QMP monitor
-> monitor disconnect visits all fdsets on monitor and turns off monitor
in-use flag for fdset
8. connecting the QMP monitor
-> monitor connect visits all fdsets on monitor and turns on monitor
in-use flag for fdset
9. query-fdsets
-> returns all fdsets and fds that don't have remove flag on
QMP command examples
--------------------
-> { "execute": "add-fd", "arguments": {
"fdset": 1 } }
<- { "return": { "fdset": 1, "fd": 3 } }
-> { "execute": "remove-fd", "arguments": {
"fdset": 1, "fd": 3 } }
<- { "return": {} }
-> { "execute":"query-fdsets" } =>
<- { "return" : { "fdsets": [
{ "name": "fdset1",
"fds": [ { "fd": 4, "removed": false } ],
"refcount": 1,
"monitor": true },
{ "name": "fdset2",
"fds": [ { "fd": 5, "removed": false },
{ "fd": 6, "removed": true } ],
"refcount": 1,
"monitor": true } }
--
Regards,
Corey