[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Pavel Butsykin
Subject: Re: [Qemu-devel] [PATCH 02/10] qcow2: add qcow2_co_write_compressed
Date: Mon, 30 May 2016 15:58:01 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0

On 30.05.2016 12:12, Pavel Butsykin wrote:
On 27.05.2016 20:33, Stefan Hajnoczi wrote:
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,

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.

Thanks for the explanation, but the backslash is used more for the
person as a marker a line break. The current coding style misses this
point, but I can remove the backslash, because I don't think it's
something important :)

+    if (!cluster_offset) {
+        qemu_co_mutex_unlock(&s->lock);
+        ret = -EIO;
+        goto fail;
+    }
+    cluster_offset &= s->cluster_offset_mask;

-        ret = bdrv_pwrite(bs->file->bs, cluster_offset, out_buf,
-        if (ret < 0) {
-            goto fail;
-        }
+    ret = qcow2_pre_write_overlap_check(bs, 0, cluster_offset,
+    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);
+    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

Sorry, but it seems this will never happen, because the second write
will not pass this check:

uint64_t qcow2_alloc_compressed_cluster_offset(BlockDriverState *bs,
                                               uint64_t offset,
                                               int compressed_size)
/* Compression can't overwrite anything. Fail if the cluster was already
     * allocated. */
    cluster_offset = be64_to_cpu(l2_table[l2_index]);
    if (cluster_offset & L2E_OFFSET_MASK) {
        qcow2_cache_put(bs, s->l2_table_cache, (void**) &l2_table);
        return 0;

As you can see we can't do the compressed write in the already allocated

      Sector 1

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


      Sector 1

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

I agree, there is really a race.. Thank you, this is a very good point!

reply via email to

[Prev in Thread] Current Thread [Next in Thread]