From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:50605) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoIbB-0007pi-CA for qemu-devel@nongnu.org; Mon, 09 Jul 2012 14:22:19 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1SoIb8-00053v-Uu for qemu-devel@nongnu.org; Mon, 09 Jul 2012 14:22:16 -0400 Received: from e5.ny.us.ibm.com ([32.97.182.145]:39720) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1SoIb8-00053E-PW for qemu-devel@nongnu.org; Mon, 09 Jul 2012 14:22:14 -0400 Received: from /spool/local by e5.ny.us.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Mon, 9 Jul 2012 14:22:07 -0400 Received: from d01relay04.pok.ibm.com (d01relay04.pok.ibm.com [9.56.227.236]) by d01dlp01.pok.ibm.com (Postfix) with ESMTP id 73D4D38C8072 for ; Mon, 9 Jul 2012 14:21:39 -0400 (EDT) Received: from d03av06.boulder.ibm.com (d03av06.boulder.ibm.com [9.17.195.245]) by d01relay04.pok.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q69ILbLe202392 for ; Mon, 9 Jul 2012 14:21:37 -0400 Received: from d03av06.boulder.ibm.com (loopback [127.0.0.1]) by d03av06.boulder.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q69ILBYf020000 for ; Mon, 9 Jul 2012 12:21:11 -0600 Message-ID: <4FFB20D4.2000209@linux.vnet.ibm.com> Date: Mon, 09 Jul 2012 14:20:04 -0400 From: Corey Bryant MIME-Version: 1.0 References: <1340390174-7493-1-git-send-email-coreyb@linux.vnet.ibm.com> <20120626091004.GA14451@redhat.com> <4FE9A0F0.2050809@redhat.com> <20120626175045.2c7011b3@doriath.home> <4FEA37A9.10707@linux.vnet.ibm.com> <4FEA3D9C.8080205@redhat.com> <4FF21A67.8010100@linux.vnet.ibm.com> <4FF31265.1000308@linux.vnet.ibm.com> <4FF316C9.5020100@redhat.com> <4FF31CFD.7030508@linux.vnet.ibm.com> <4FF325C8.4060401@redhat.com> <4FF33004.5030909@linux.vnet.ibm.com> <4FF33349.10404@redhat.com> <4FF3381D.40101@linux.vnet.ibm.com> <4FF3FA22.6090400@redhat.com> <4FF5AD90.8000305@linux.vnet.ibm.com> <20120709110510.12214347@doriath.home> <4FFAF334.9000807@linux.vnet.ibm.com> In-Reply-To: <4FFAF334.9000807@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 0/7] file descriptor passing using pass-fd List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Luiz Capitulino Cc: Kevin Wolf , aliguori@us.ibm.com, stefanha@linux.vnet.ibm.com, libvir-list@redhat.com, qemu-devel@nongnu.org, pbonzini@redhat.com, Eric Blake On 07/09/2012 11:05 AM, Corey Bryant wrote: > > > On 07/09/2012 10:05 AM, Luiz Capitulino wrote: >> On Thu, 05 Jul 2012 11:06:56 -0400 >> Corey Bryant 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 } } > Here's a minor correction to query-fdsets. s/name/fdset and I changed fdset to an int to stay in sync with add-fd and remove-fd. -> { "execute":"query-fdsets" } => <- { "return" : { "fdsets": [ { "fdset": 1, "fds": [ { "fd": 4, "removed": false } ], "refcount": 1, "monitor": true }, { "fdset": 2, "fds": [ { "fd": 5, "removed": false }, { "fd": 6, "removed": true } ], "refcount": 1, "monitor": true } } -- Regards, Corey