Re: [Qemu-devel] [PATCH 02/10] qcow2: add qcow2_co_write_compressed

[Stefan Hajnoczi]

On Sat, May 14, 2016 at 03:45:50PM +0300, Denis V. Lunev wrote:
> +    qemu_co_mutex_lock(&s->lock);
> +    cluster_offset = \
> +        qcow2_alloc_compressed_cluster_offset(bs, sector_num << 9, out_len);

The backslash isn't necessary for wrapping lines in C.  This kind of
thing is only necessary in languages like Python where the grammar is
whitespace sensistive.

The C compiler is happy with an arbitrary amount of whitespace
(newlines) in the middle of a statement.  The backslash in C is handled
by the preprocessor: it joins the line.  That's useful for macro
definitions where you need to tell the preprocessor that several lines
belong to one macro definition.  But it's not needed for normal C code.

> +    if (!cluster_offset) {
> +        qemu_co_mutex_unlock(&s->lock);
> +        ret = -EIO;
> +        goto fail;
> +    }
> +    cluster_offset &= s->cluster_offset_mask;
>  
> -        BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
> -        ret = bdrv_pwrite(bs->file->bs, cluster_offset, out_buf, out_len);
> -        if (ret < 0) {
> -            goto fail;
> -        }
> +    ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset, out_len);
> +    qemu_co_mutex_unlock(&s->lock);
> +    if (ret < 0) {
> +        goto fail;
>      }
>  
> +    iov = (struct iovec) {
> +        .iov_base   = out_buf,
> +        .iov_len    = out_len,
> +    };
> +    qemu_iovec_init_external(&hd_qiov, &iov, 1);
> +
> +    BLKDBG_EVENT(bs->file, BLKDBG_WRITE_COMPRESSED);
> +    ret = bdrv_co_pwritev(bs->file->bs, cluster_offset, out_len, &hd_qiov, 
> 0);

There is a race condition here:

If the newly allocated cluster is only partially filled by compressed
data then qcow2_alloc_compressed_cluster_offset() remembers that more
bytes are still available in the cluster.  The
qcow2_alloc_compressed_cluster_offset() caller will continue filling the
same cluster.

Imagine two compressed writes running at the same time.  Write A
allocates just a few bytes so write B shares a sector with the first
write:

     Sector 1
  |AAABBBBBBBBB|

The race condition is that bdrv_co_pwritev() uses read-modify-write (a
bounce buffer).  If both requests call bdrv_co_pwritev() around the same
time then the following could happen:

     Sector 1
  |000BBBBBBBBB|

or:

     Sector 1
  |AAA000000000|

It's necessary to hold s->lock around the compressed data write to avoid
this race condition.

signature.asc
Description: PGP signature