Re: [Qemu-devel] [PATCH] block/vdi: Fix locking for parallel requests

[Max Reitz]

On 2015-02-17 at 16:49, Paolo Bonzini wrote:
> Cc: qemu-stable <address@hidden>

Right, I forgot that. Thanks!

Max

> On 17/02/2015 22:33, Max Reitz wrote:
> > Concurrently modifying the bmap is not a good idea; this patch adds a
> > lock for it. See https://bugs.launchpad.net/qemu/+bug/1422307 for what
> > can go wrong without.
> >
> > Signed-off-by: Max Reitz <address@hidden>
> > ---
> >   block/vdi.c | 23 ++++++++++++++++++++---
> >   1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/block/vdi.c b/block/vdi.c
> > index 74030c6..c5ff428 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,
> > @@ -619,6 +624,9 @@ static int vdi_co_write(BlockDriverState *bs,
> >                  n_sectors, sector_num);
> >   
> >           /* prepare next AIO request */
> > +        if (!block) {
> > +            qemu_co_mutex_lock(&s->bmap_lock);
> > +        }
> >           bmap_entry = le32_to_cpu(s->bmap[block_index]);
> >           if (!VDI_IS_ALLOCATED(bmap_entry)) {
> >               /* Allocate new block and write to it. */
> > @@ -641,9 +649,13 @@ static int vdi_co_write(BlockDriverState *bs,
> >                      (s->block_sectors - n_sectors - sector_in_block) * 
> > SECTOR_SIZE);
> >               ret = bdrv_write(bs->file, offset, block, s->block_sectors);
> >           } else {
> > -            uint64_t offset = s->header.offset_data / SECTOR_SIZE +
> > -                              (uint64_t)bmap_entry * s->block_sectors +
> > -                              sector_in_block;
> > +            uint64_t offset;
> > +
> > +            qemu_co_mutex_unlock(&s->bmap_lock);
> > +
> > +            offset = s->header.offset_data / SECTOR_SIZE +
> > +                     (uint64_t)bmap_entry * s->block_sectors +
> > +                     sector_in_block;
> >               ret = bdrv_write(bs->file, offset, buf, n_sectors);
> >           }
> >   
> > @@ -656,6 +668,9 @@ static int vdi_co_write(BlockDriverState *bs,
> >   
> >       logout("finished data write\n");
> >       if (ret < 0) {
> > +        if (block) {
> > +            qemu_co_mutex_unlock(&s->bmap_lock);
> > +        }
> >           return ret;
> >       }
> >   
> > @@ -688,6 +703,8 @@ static int vdi_co_write(BlockDriverState *bs,
> >           logout("will write %u block map sectors starting from entry %u\n",
> >                  n_sectors, bmap_first);
> >           ret = bdrv_write(bs->file, offset, base, n_sectors);
> > +
> > +        qemu_co_mutex_unlock(&s->bmap_lock);
> >       }
> >   
> >       return ret;