[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v12 15/16] qcow2: Force "no other writer" lock o
From: |
Fam Zheng |
Subject: |
Re: [Qemu-devel] [PATCH v12 15/16] qcow2: Force "no other writer" lock on bs->file |
Date: |
Wed, 8 Feb 2017 23:00:08 +0800 |
User-agent: |
Mutt/1.7.1 (2016-10-04) |
On Wed, 02/08 15:33, Max Reitz wrote:
> 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.
I agree. I believe BDRV_O_SHARE_RW is replaced by op blocker primitives but I've
not checked yet. Tomorrow I'll take a look at Kevin's branch and see if it is
easy enough to softly base on top of it.
>
> > 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.
Sounds good!
Fam