qemu-devel
[Top][All Lists]
Advanced

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

Re: [Qemu-devel] [PATCH 5/8] block: add drive-mirror command


From: Paolo Bonzini
Subject: Re: [Qemu-devel] [PATCH 5/8] block: add drive-mirror command
Date: Mon, 16 Apr 2012 09:29:38 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120329 Thunderbird/11.0.1

Il 14/04/2012 01:08, Eric Blake ha scritto:
> On 04/13/2012 10:23 AM, Paolo Bonzini wrote:
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
> 
>> +    flags = bs->open_flags | BDRV_O_RDWR;
>> +    source = bs->backing_hd;
> 
>> +    /* create new image w/backing file */
>> +    if (full && mode != NEW_IMAGE_MODE_EXISTING) {
>> +        assert(format && drv);
>> +        bdrv_get_geometry(bs, &size);
>> +        size *= 512;
>> +        ret = bdrv_img_create(target, format,
>> +                              NULL, NULL, NULL, size, flags);
> 
> This side of the branch forces the size (which makes sense, since there
> is no backing file to probe it from),...
> 
>> +    } else {
>> +        switch (mode) {
>> +        case NEW_IMAGE_MODE_EXISTING:
>> +            ret = 0;
>> +            break;
>> +        case NEW_IMAGE_MODE_ABSOLUTE_PATHS:
>> +            ret = bdrv_img_create(target, format,
>> +                                  source->filename,
>> +                                  source->drv->format_name,
>> +                                  NULL, -1, flags);
> 
> ...but this side passes -1 (which I assume means to probe the backing
> file for its size, and reuse that size).  But is this always right, or
> shouldn't this side of the branch _also_ be calling bdrv_get_geometry
> and passing in an explicit size?

This could be an idea---and for blockdev_snapshot_sync too, where it
would avoid a useless probe and opening the same file twice.  It's not a
bigger problem than for snapshots, especially since now we open the file
for read only.  It could be a small problem in case the source backing
file is smaller than the source itself.  I don't know if that's possible.

> Should we be using BDRV_O_NO_BACKING
> to avoid potential problems in temporarily reopening a file that we
> already have open due to source?

Note that bdrv_img_create does not use BDRV_O_NO_BACKING and does not
open the backing file other than for probing.  mirror_start does use
BDRV_O_NO_BACKING.

> Am I correct that bdrv_img_create will fail if I attempt to use a driver
> that doesn't support backing files (think raw) but include a request to
> set the backing file?  (Put another way, I'm wondering whether libvirt
> can trust qemu to do error detection, or whether libvirt must filter out:
> virDomainBlockRebase(, _COPY | _SHALLOW | _COPY_RAW)
> as reasonable on an existing raw image, but as an error if the existing
> image already uses a backing file.

QEMU will fail here, if I understand what you mean:

    if (base_filename) {
        if (set_option_parameter(param, BLOCK_OPT_BACKING_FILE,
                                 base_filename)) {
            error_report("Backing file not supported for file format '%s'",
                         fmt);
            ret = -EINVAL;
            goto out;
        }
    }

> Just to make sure: if I pass in an existing file, then it is expected
> that the data in that image is either identical to the data of the
> backing file (if full==false) or is conceptually uninitialized (if
> fall=true).  We document that there might be rare cases of wanting to
> alter contents - I'm assuming that one such case would be to pass in an
> existing file with a larger size than the source, such that when we then
> do disk-reopen, we've achieved a poor-man's block_resize.  Does the code
> react well to the existing destination having a different size than the
> source?

It doesn't care.  It will also allow a smaller destination as long as
the "missing" sectors are unallocated in the source.

Paolo



reply via email to

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