Re: [Qemu-devel] [PATCH v12 15/16] qcow2: Force "no other writer" lock on bs->file

[Max Reitz]

On 23.01.2017 13:30, Fam Zheng wrote:
> Writing to the same qcow2 file from two QEMU processes at the same time
> will never work correctly, so disable it even when the caller specifies
> BDRV_O_RDWR.
> 
> Other formats are less vulnerable because they don't have internal
> snapshots thus qemu-img is less often misused to create live snapshot.

Hm, OK, reasonable. Also reasonable since we can just wait with those
until we have op blockers.

Which brings me to the op blocker point. I don't know the exact
influence Kevin's patches will have on this series, but I'd imagine they
mostly change where the BDRV_O_SHARE_RW flag comes from or whether we
need that flag at all. Therefore, I personally don't mind the order in
which your series land in master.

> Signed-off-by: Fam Zheng <address@hidden>
> ---
>  block/qcow2.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 96fb8a8..879361a 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -1177,6 +1177,17 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>          }
>      }
>  
> +    if ((flags & BDRV_O_SHARE_RW) && (flags & BDRV_O_RDWR)) {
> +        /* Shared write is never a good idea for qcow2, override it.
> +         * XXX: Use permission propagation and masking mechanism in op 
> blockers
> +         * API once it's there. */
> +        ret = bdrv_reopen(bs->file->bs, flags & ~BDRV_O_SHARE_RW, 
> &local_err);
> +        if (ret) {
> +            error_propagate(errp, local_err);
> +            goto fail;
> +        }
> +    }
> +

Good in principle, but I don't think it should be at the bottom of this
function, especially not after "Repair image if dirty". I think it would
be good to put this right at the start of qcow2_open(), actually.

Max

>  #ifdef DEBUG_ALLOC
>      {
>          BdrvCheckResult result = {0};
>

signature.asc
Description: OpenPGP digital signature