Re: [Qemu-devel] [PATCH for-2.7 v2 04/17] block: Introduce image file locking

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4036 bytes --]

On 15.04.2016 05:27, Fam Zheng wrote:
> Block drivers can implement this new operation .bdrv_lockf to actually lock the
> image in the protocol specific way.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                   | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/block/block_int.h | 12 ++++++++++++
>  2 files changed, 54 insertions(+)

I'm prepared for everyone hating this idea, but I'm just bringing it up
so I can always say I did bring it up.

Heads up: This will be about qcow2 locking again.

Relax, though, it won't be about how much better qcow2 locking is better
than protocol locking.

Now that you know this feel free to drop out.

This patch implements locking by just trying to lock every single BDS
that is being opened. While it may fulfill its purpose, I don't think
that is what we actually want.

What we want is the following: qemu has a BDS graph. It is basically a
forest of trees. It may be a bit more complicated (DAGs instead of
trees), but let's just assume it is.

What we want to protect are leaves in this tree. Every leaf basically
corresponds to a physical resource such as a file or an NBD connection.
Every leaf is driven by a protocol block driver. We want to protect
these physical resources from concurrent access.

Ideally, we can just protect the physical resource itself. This works
for raw-posix, this works for gluster, this works for raw-win32, and
probably some other protocols, too. But I guess it won't work for all
protocols, and even if it does, it would need to be implemented.

But we can protect leaves in the BDS forest by locking non-leaves also:
If you lock a qcow2 node, all of its "file" subtree will be protected;
normally, that's just a single leaf.

Therefore, I think the ideal approach would be for each BDS tree that is
to be created we try to lock all of its leaves, and if that does not
work for some, we walk up the tree and try to lock inner nodes (e.g.
format BDSs which then use format locking) so that the leaves are still
protected even if their protocol does not support that.

This could be implemented like this: Whenever a leaf BDS is created, try
to lock it. If we can't, leave some information to the parent node that
its child could not be locked. Then, the parent will evaluate this
information and try to act upon it. This then recurses up the tree. Or,
well, down the tree, considering that in most natural trees the root is
at the bottom.


We could just implement qcow2 locking on top of this series as it is,
but this would result in qcow2 files being locked even if their files'
protocol nodes have been successfully locked. That would be superfluous
and we'd have all the issues with force-unlocking qcow2 files we have
discussed before.


So what am I saying? I think that it makes sense to consider format
locking as a backup alternative to protocol locking in case the latter
is not possible. I think it is possible to implement both using the same
framework.

I don't think we need to worry about the actual implementation of format
locking now. But I do think having a framework which supports both
format and protocol locking is possible and would be nice to have.

Such a framework would require more effort, however, than the basically
brute-force "just lock everything" method presented in this patch. Don't
get me wrong, this method here works for what it's supposed to do (I
haven't reviewed it yet, though), and it's very reasonable if protocol
locking is all we intend to have. I'm just suggesting that maybe we do
want to have more than that.


All in all, I won't object if the locking framework introduced by this
series is not supposed to and does not work with format locking. It can
always be added later if I really like it so much, and I can definitely
understand if it appears to be too much effort for basically no gain
right now.

As I said above, I just brought this up so I brought it up. :-)

Max


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]