Re: [Qemu-devel] [PATCH] Introduce cache images for the QCOW2 format

[Alex Bligh <alex@alex.org.uk>]

--On 13 August 2013 19:03:56 +0200 Kaveh Razavi <kaveh@cs.vu.nl> wrote:

> This patch introduces a block-level caching mechanism by introducing a
> copy-on-read image that supports quota and goes in between the base
> image and copy-on-write image. This cache image can either be stored on
> the nodes that run VMs or on a storage device that can handle random
> access well (e.g. memory, SSD, etc.).

What is this cache keyed on and how is it invalidated? Let's say a
2 VM on node X boot with backing file A. The first populates the cache,
and the second utilises the cache. I then stop both VMs, delete
the derived disks, and change the contents of the backing file. I then
boot a VM using the changed backing file on node X and node Y. I think
node Y is going to get the clean backing file. However, how does node
X know not to use the cache? Would it not be a good idea to check
(at least) the inode number and the mtime of the backing file correspond
with values saved in the cache, and if not the same then ignore the
cache?

> +    /* backing files always opened read-only except for cache images,
> +     * we first open the file with RDWR and check whether it is a cache
> +     * image. If so, we leave the RDWR, if not, we re-open read-only.
> +     */

This seems like the less safe way of doing it. Why not open RO and check
whether it is a cache image, and if so reopen RDWR. That would mean
you never open a backing file that isn't a cache image RDWR even for
only a second? That sounds safer to me, and is also the least change
for the existing code path.

> @@ -840,7 +886,6 @@ static coroutine_fn int
> qcow2_co_writev(BlockDriverState *bs,      qemu_co_mutex_lock(&s->lock);
>
>      while (remaining_sectors != 0) {
> -
>          l2meta = NULL;
>
>          trace_qcow2_writev_start_part(qemu_coroutine_self());

Pointless whitespace change.

-- 
Alex Bligh