Re: [Qemu-devel] [PATCH 3/4] Block: readonly changes

[Naphtali Sprei]

Jamie Lokier wrote:
> Naphtali Sprei wrote:
>> Open backing file for read-only
>> During commit upgrade to read-write and back at end to read-only
> 
>> +    if (ro) { /* re-open as RO */
>> +        bs_ro = bdrv_new("");
>> +        ret = bdrv_open2(bs_ro, bs->backing_hd->filename,  
>> bs->backing_hd->open_flags & ~BDRV_O_RDWR, NULL);
>> +        if (ret < 0) {
>> +            bdrv_delete(bs_ro);
>> +            return -EACCES;
>> +        }
>> +        bdrv_close(bs->backing_hd);
>> +        qemu_free(bs->backing_hd);
>> +        bs->backing_hd = bs_ro;
>> +        bs->backing_hd->keep_read_only = 0;
>> +    }
> 
> I think the general idea is perfect.
> 
> A couple of concerns come to mind.
> 
> 1. When changing read-write to read-only, if the backing file is a complex
>    format like qcow2 (or any others), is it possible for this bdrv_open2()
>    to read metadata such as format indexes, and even data, _before_
>    all changes maintained by bs->backing_hd have been written to the file?
> 
>    (If the complex formats were like real filesystems and had a "mounted"
>    flags as real filesystems tend to, then it would be an issue, but I'm
>    not aware of any of them doing that.)
> 
>    Are there any such issues when switching from read-only to read-write
>    earlier?  (It seems unlikely).
> 

Good question. I looked at some of the formats (qcow, qcow2, vmdk) and didn't 
see
anything problematic, since in the close function I didn't see any changes to 
the real file,
only in-memory data and memory free.
But an answer from an expert would help.

> 2. Secondly, what if the re-open gets a different file (testable with
>    fstat()).  I know, you get what you deserve if you rename files, but
>    still, do any of the formats which use backing files have a UUID check
>    or something to confirm they are using the right backing file, which
>    might be subverted by this?

I didn't see any such checking/validation.
It seems that handling such cases will complicate things more than you gain.

> 
> 3. What about the bdrv file/device-locking which was actively looking
>    like it might get in a couple of months ago.  Did it get in, and if
>    so does it conflict with this upgrade pattern?

AFAIK, the locks thread terminated, don't think anything committed.
But surely, there's a tight relationship between read-only/locks and sharing.

> 
> Thanks!
> -- Jamie

 Thanks,

  Naphtali