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

[Stefan Hajnoczi]

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.)

Stefan

pgp6q0jO6mup_.pgp
Description: PGP signature