Re: [Qemu-block] [PATCH v6 03/22] blockdev: Add and parse "lock-mode" option for image locking

[Kevin Wolf]

Am 08.07.2016 um 04:56 hat Fam Zheng geschrieben:
> On Tue, 07/05 15:37, Kevin Wolf wrote:
> > Am 17.06.2016 um 11:23 hat Kevin Wolf geschrieben:
> > > Am 03.06.2016 um 10:48 hat Fam Zheng geschrieben:
> > > > Respect the locking mode from CLI or QMP, and set the open flags
> > > > accordingly.
> > > > 
> > > > Signed-off-by: Fam Zheng <address@hidden>
> > > > Reviewed-by: Max Reitz <address@hidden>
> > 
> > > This is the wrong level to implement the feature. You would only be able
> > > to configure the locking on the top level image this way (because what
> > > we're doing here is BB, not BDS configuration). If you want to allow
> > > configuration per node, you need to put the options into
> > > bdrv_runtime_opts and interpret them in bdrv_open_common().
> > > 
> > > Otherwise we would have to specify in the blockdev-add documentation
> > > that this works only on the top level, but I don't see a reason for
> > > such a restriction.
> > 
> > And actually, after some more thinking about block device configuration,
> > I'm not sure any more whether letting the user configure this on the
> > node level, at least as the primary interface.
> > 
> > A node usually knows by itself what locking mode it needs to request
> > from its children, depending on the locking mode that the parent node
> > requested for it. It could be passing down the locking mode (raw
> > format), it could require a stricter locking mode (non-raw formats never
> > work with r/w sharing) or it could even be less strict (backing files
> > are normally ro/ and can therefore be shared, even if the guest can't
> > share its image).
> > 
> > The real origin of the locking requirement is the top level. So maybe
> > the right interface for guest devices is adding a qdev option that tells
> > whether the guest can share the image. For NBD servers, we'd add a QMP
> 
> I think most block devices are not designed to share the data, so in general
> it's hard to imagine this as a device property.

Well, it's really a guest OS (or even guest application) property, but
obviously that doesn't exist. And the device is the qemu component that
is the closest to the guest. We generally have options about behaviour
that the guest expects at the device level.

> > option that tells whether client can share the image. And starting from
> > these requirements, the locking mode would propagate through the graph,
> > with each node deciding what it needs to request from its children in
> > order to achieve the protection that its parent requested.
> > 
> > And at this point I start wondering... Doesn't this look an awful lot
> > like op blockers? (The new ones.) Should image locking be integrated
> > there?
> > 
> > I still see a (limited) use for the node-level configuration: The user
> > might want to request a stricter locking mode than is necessary because
> > they foresee an operation that will change the requirements (e.g. commit
> > to a backing file) and they don't want to risk failure then.
> > 
> > Any opinions?
> 
> Who is going to enable the default auto lock with an unattached (no BB or no
> device) image, such as the qemu-img case?  Lock mode there needs to be
> configurable too, but moving the option away from the BB/BDS makes this
> trickier to do.

Unattached BDSes don't get I/O. qemu-img does use a BB.

Do you actually need to configure the locking mode for qemu-img or is
just a switch for ignoring locks enough? Your -L could be implemented
more or less the same way as it is now. You got a user option and you
pass it down to the lower layers. The exact way of how you pass it down
(flags vs. possibly some op blocker thing) could change, but it doesn't
really change much conceptually.

Kevin