From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:34104) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWOXu-0006co-Ka for qemu-devel@nongnu.org; Tue, 23 Sep 2014 07:46:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XWOXo-0003fH-Dv for qemu-devel@nongnu.org; Tue, 23 Sep 2014 07:46:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:65504) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XWOXo-0003er-7C for qemu-devel@nongnu.org; Tue, 23 Sep 2014 07:46:08 -0400 Date: Tue, 23 Sep 2014 13:45:56 +0200 From: Kevin Wolf Message-ID: <20140923114556.GF3871@noname.str.redhat.com> References: <1410891148-28849-1-git-send-email-armbru@redhat.com> <1410891148-28849-4-git-send-email-armbru@redhat.com> <20140922145955.GD4026@noname.redhat.com> <87a95r4nt3.fsf@blackfin.pond.sub.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87a95r4nt3.fsf@blackfin.pond.sub.org> Subject: Re: [Qemu-devel] [PATCH v3 03/23] block: Connect BlockBackend to BlockDriverState List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: benoit.canet@nodalink.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, mreitz@redhat.com Am 22.09.2014 um 18:34 hat Markus Armbruster geschrieben: > Kevin Wolf writes: > > > Am 16.09.2014 um 20:12 hat Markus Armbruster geschrieben: > >> diff --git a/include/block/block_int.h b/include/block/block_int.h > >> index 8d86a6c..14e0b7c 100644 > >> --- a/include/block/block_int.h > >> +++ b/include/block/block_int.h > >> @@ -324,6 +324,8 @@ struct BlockDriverState { > >> BlockDriver *drv; /* NULL means no media */ > >> void *opaque; > >> > >> + BlockBackend *blk; /* owning backend, if any */ > >> + > >> void *dev; /* attached device model, if any */ > >> /* TODO change to DeviceState when all users are qdevified */ > >> const BlockDevOps *dev_ops; > > > > Just to make sure that we agree on where we're going: This makes the > > assumption that a BDS has at most one BB that owns it. > > Yes. > > > Which is not the > > final state that we want to have, so this will have to go away later. > > I don't know. Can you explain why you think we're going to want > multiple BBs? We already agreed that we'll have multiple parents for a BDS, for scenarios like having an NBD server on a snapshot or sharing backing files, potentially also some block jobs. The question is whether among these multiple parents we want to have a limitation to one BlockBackend, forbidding e.g. an NBD server on the active layer. This would be a problem for live storage migration if we don't want the NBD server to reuse the same BB as the guest device. More generally, if we can indirectly have multiple BBs on a single BDS by putting a filter in between, do we have good reasons to forbid having them attached directly? > > (Where "later" isn't necessarily part of this series.) > > > > For now, the use of the field is limited to callbacks and > > bdrv_get_device_name(). Callbacks could always only serve a single > > device, so nothing became worse here. > > In *this* patch, member blk is only read in bdrv_swap(), which asserts > it's null. Later on in the series, it gets indeed used as you describe. Yes, my "now" depends on context and either refers to the patch I'm commenting on or the end of the series. In most cases when I see something that I feel is worth having a closer look, the first thing I do is looking at the fully applied series. > PATCH 22 puts it to use for BlockDevOps callbacks. The patch moves the > callbacks from BDS to BB. I hope you'll agree that's where they belong. > > Naturally, the *calls* of the callbacks remain where they are, in > block.c. They get updated like this: > > - bdrv_dev_FOO(bs, ARGS) > + if (bs->blk) { > + blk_dev_FOO(bs->blk ARGS) > + } Yes, as I said, this is fine for now. When we allow multiple BBs, we'll have to turn it into something like notifier lists, but that can wait. > PATCH 08 uses it to eliminate BDS member device_name[]. > > > I'm not entirely sure about bdrv_get_device_name(), whether it needs to > > go or to be rewritten to get the name of any BB pointing to it (I > > suspect for most callers we want to replace it by something that uses > > node-name by default if there is one and only fall back to BB names if > > there isn't), but that's not an issue to block this patch. > > I agree users of bdrv_get_device_name() need to be examined, and the > ones that really want a BDS name should probably be changed to use the > BDS name (a.k.a. node-name) and fall back to the BB name. > > This series makes this need more visible, by emphasizing the > distinctness of the two names. > > Aside: which one to fall back to if we have multiple BBs? My first attempt would be "any", and in cases where this isn't good enough, you can't use a fallback at all. > > What I would consider, however, is adding a TODO comment that tells > > people that this field needs to go and if you need to use it, something > > is wrong with your design (which happens to be true for the existing > > design of some code). > > For the device callbacks, we need a way to find the BB. If multiple BBs > can sit on top of the same BDS, we need to find the one with a device > models attached. Ot even the ones, if we permit that. > > Let's discuss this a bit, and depending on what we learn, add a suitable > comment. Possibly on top. Are you sure that nothing else than device models can be interested in callbacks? I expect that whatever block layer user we have, they will always be interested in resizes, for example. Media change might also not be entirely uninteresting, though in most cases what other users want is probably a blocker. Kevin