qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH v3 03/23] block: Connect BlockBackend to BlockDr


From: Kevin Wolf
Subject: Re: [Qemu-devel] [PATCH v3 03/23] block: Connect BlockBackend to BlockDriverState
Date: Tue, 23 Sep 2014 13:45:56 +0200
User-agent: Mutt/1.5.21 (2010-09-15)

Am 22.09.2014 um 18:34 hat Markus Armbruster geschrieben:
> Kevin Wolf <address@hidden> 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



reply via email to

[Prev in Thread] Current Thread [Next in Thread]