|
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, 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.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; - 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:
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 cluster.
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.I agree, there is really a race.. Thank you, this is a very good point!
[Prev in Thread] | Current Thread | [Next in Thread] |