From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:49381) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TDtQq-00008T-Dr for qemu-devel@nongnu.org; Tue, 18 Sep 2012 04:45:28 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TDtQk-00070q-BU for qemu-devel@nongnu.org; Tue, 18 Sep 2012 04:45:24 -0400 Received: from mx1.redhat.com ([209.132.183.28]:32453) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TDtQk-0006zH-2b for qemu-devel@nongnu.org; Tue, 18 Sep 2012 04:45:18 -0400 Message-ID: <50583498.5020100@redhat.com> Date: Tue, 18 Sep 2012 10:45:12 +0200 From: Kevin Wolf MIME-Version: 1.0 References: <1346079626-16386-1-git-send-email-pbonzini@redhat.com> <504A17B6.6020703@redhat.com> <504A1CBB.6000800@redhat.com> <50575318.5010001@redhat.com> In-Reply-To: <50575318.5010001@redhat.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] ping Re: [RFC PATCH 00/13] Embedded NBD server List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: stefanha@gmail.com, qemu-devel@nongnu.org Am 17.09.2012 18:43, schrieb Paolo Bonzini: > Il 07/09/2012 18:11, Kevin Wolf ha scritto: >> I was planning to review it in more detail next week, but I just had a >> quick look. I'm not sure if automatically shutting down the NBD server >> when the guest stops using it is always right (for removable media it >> could even be an eject from the guest), > > Yes, the removable media case could be a bit too eager. Note however > that a guest-triggered eject doesn't do bdrv_close, only a > user-triggered eject does, and that's blocked by bdrv_in_use. > > Luckily removable media are usually not too interesting, so a slightly > suboptimal behavior is okay as long as it does not break the important > use cases---mostly migration without shared storage, where also > uninteresting images have to be mirrored or exposed via NBD. Those > should be covered by bdrv_in_use. It sounds like it could be acceptable, yes. But what's even the motivation to close the server on bdrv_close? The commit message is a bit... well, not just terse, but even empty. The standard case for closing images is that qemu exits. In this case, the NBD server would automatically exit as well. An interesting case for the NBD server would be when the migration has completed; but do we even get a bdrv_close there? >> but introducing a notifier list >> doesn't look too bad. We can probably use it for other things that are >> currently hardcoded in bdrv_close() with some if statements, like >> disabling I/O throttling, cancelling a block job, etc. > > Yes, though a lot of these could be moved to "filters" and use whatever > filter-specific method is there (e.g. a filter bdrv_close). This > circles back to the question of whether bdrv_close kills filters or only > the base image... Note that after completing the refactoring, we'll only have one combined bdrv_close/delete function and so there won't be BlockDriverStates that are closed. In this case, I think it's quite obvious that not closing the filters wouldn't make any sense. Kevin