Re: [Qemu-stable] [PATCH v2] block/vdi: Add locking for parallel requests

[Max Reitz]

On 2015-02-27 at 11:57, Stefan Hajnoczi wrote:
> On Fri, Feb 27, 2015 at 09:05:47AM -0500, Max Reitz wrote:
> > Concurrently modifying the bmap does not seem to be a good idea; this patch adds
> > a lock for it. See https://bugs.launchpad.net/qemu/+bug/1422307 for what
> > can go wrong without.
> >
> > Cc: qemu-stable <address@hidden>
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> > v2:
> > - Make the mutex cover vdi_co_write() completely [Kevin]
> > - Add a TODO comment [Kevin]
> > ---
> >   block/vdi.c | 11 +++++++++++
> >   1 file changed, 11 insertions(+)
> >
> > diff --git a/block/vdi.c b/block/vdi.c
> > index 74030c6..f5f42ef 100644
> > --- a/block/vdi.c
> > +++ b/block/vdi.c
> > @@ -51,6 +51,7 @@
> >   
> >   #include "qemu-common.h"
> >   #include "block/block_int.h"
> > +#include "block/coroutine.h"
> >   #include "qemu/module.h"
> >   #include "migration/migration.h"
> >   
> > @@ -196,6 +197,8 @@ typedef struct {
> >       /* VDI header (converted to host endianness). */
> >       VdiHeader header;
> >   
> > +    CoMutex bmap_lock;
> > +
> >       Error *migration_blocker;
> >   } BDRVVdiState;
> >   
> > @@ -498,6 +501,8 @@ static int vdi_open(BlockDriverState *bs, QDict *options, int flags,
> >           goto fail_free_bmap;
> >       }
> >   
> > +    qemu_co_mutex_init(&s->bmap_lock);
> > +
> >       /* Disable migration when vdi images are used */
> >       error_set(&s->migration_blocker,
> >                 QERR_BLOCK_FORMAT_FEATURE_NOT_SUPPORTED,
> > @@ -607,6 +612,9 @@ static int vdi_co_write(BlockDriverState *bs,
> >   
> >       logout("\n");
> >   
> > +    /* TODO: Figure out why this is necessary */
> > +    qemu_co_mutex_lock(&s->bmap_lock);
> If we don't know why bmap_lock works, it would be more approprate to
> take the same approach as VMDK and VHDX where there is a simply s->lock
> that protects all reads and writes.  That way we know for sure there is
> no parallel I/O going on.
>
> (Since the problem is not understood, maybe reads in parallel with
> writes could also cause problems.  Better to really do a coarse lock
> instead of just bmap_lock in write.)

OK, will do.

Max