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

Am 03.07.2012 19:03, schrieb Eric Blake: >>>> 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. Good point. So pass-fd or whatever it will be called needs to returnn both fdset number and the individual fd number. >>>> 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 This is an interesting idea. I've never thought about what to do with a reconnect. It felt a bit odd at first, but now your proposal totally makes sense to me. > 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.

> 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 > > 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 Yup, apart from the comment above the examples look good to me. > 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, makes sense to me. >> 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 } ] } ] } } Looks good, this is exactly what I was thinking of. >>> 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.