Re: [Qemu-devel] [PATCH] block: Formats don't need CONSISTENT_READ with NO_IO

[Kevin Wolf]

Am 30.11.2017 um 18:09 hat Eric Blake geschrieben:
> On 11/30/2017 10:44 AM, Kevin Wolf wrote:
> > Commit 1f4ad7d fixed 'qemu-img info' for raw images that are currently
> > in use as a mirror target. It is not enough for image formats, though,
> > as these still unconditionally request BLK_PERM_CONSISTENT_READ.
> > 
> > As this permission is meaningless unless you do actual I/O on the image,
> > drop the requirement and allow 'qemu-img info' even for image formats
> > under conditions where BLK_PERM_CONSISTENT_READ can't be granted.
> > 
> > Signed-off-by: Kevin Wolf <address@hidden>
> > ---
> >   block.c | 6 +++++-
> >   1 file changed, 5 insertions(+), 1 deletion(-)
> > 
> 
> > @@ -1936,7 +1938,9 @@ void bdrv_format_default_perms(BlockDriverState *bs, 
> > BdrvChild *c,
> >           /* bs->file always needs to be consistent because of the 
> > metadata. We
> >            * can never allow other users to resize or write to it. */
> > -        perm |= BLK_PERM_CONSISTENT_READ;
> > +        if (!(flags & BDRV_O_NO_IO)) {
> > +            perm |= BLK_PERM_CONSISTENT_READ;
> 
> I thought BDRV_O_NO_IO only means we aren't doing I/O on guest-visible data,
> but doesn't stop us from reading the metadata.  The comment is telling: if
> we can read metadata, then we depend on CONSISTENT_READ for the metadata to
> be stable (even if we don't care about guest data consistency).

Yes and no. The trouble is that at the file system level we have only a
single bit to describe the consistency of the whole image throughout the
whole block driver tree.

We forbid shared BLK_PERM_CONSISTENT_READ for mirror targets (which
aren't fully populated yet) and intermediate nodes for commit (which
expose corrupted data). Both scenarios are really about the data exposed
at the format layer, the metadata stays completely consistent.

The question is what we do with this as we propagate permissions down to
the protocol layer. Strictly speaking, the file at the protocol layer is
perfectly consistent, so we might not forbid BLK_PERM_CONSISTENT_READ
there. But I think it's more useful to do it anyway so that image
locking can prevent the typical case of another process that uses qcow2
over file-posix again, where the file-posix node could in theory be
considered consistent, but the qcow2 one wouldn't.

In the end, this is just a pragmatic way to let 'qemu-img info' work
while the image is a mirror target or intermediate node for commit, but
to forbid cases where corrupted data is used.

Or would you argue that either 'qemu-img info' shouldn't be working or
reading corrupted data should be allowed in other processes?

Kevin