Re: [Qemu-devel] [PATCH v8 09/21] null: Switch to .bdrv_co_block_status()

[Eric Blake]

On 02/14/2018 06:05 AM, Kevin Wolf wrote:

> > +static int coroutine_fn null_co_block_status(BlockDriverState *bs,

> >       if (s->read_zeroes) {
> > -        return BDRV_BLOCK_OFFSET_VALID | start | BDRV_BLOCK_ZERO;
> > -    } else {
> > -        return BDRV_BLOCK_OFFSET_VALID | start;
> > +        ret |= BDRV_BLOCK_ZERO;
> >       }
> > +    return ret;
> >   }
>
> Preexisting, but I think this return value is wrong. OFFSET_VALID
> without DATA is to documented to have the following semantics:
>
>   * DATA ZERO OFFSET_VALID
>   *  f    t        t       sectors preallocated, read as zero, returned file not
>   *                        necessarily zero at offset
>   *  f    f        t       sectors preallocated but read from backing_hd,
>   *                        returned file contains garbage at offset
>
> I'm not sure what OFFSET_VALID is even supposed to mean for null.

I'm finally getting around to playing with this.

> Or in fact, what it is supposed to mean for any protocol driver, because
> normally it just means I can use this offset for accessing bs->file. But > 
> protocol drivers don't have a bs->file, so it's interesting to see that
> they still all set this flag.

More precisely, it means "I can use this offset for accessing the 
returned *file".  Format and filter drivers set *file = bs->file (ie. 
their protocol layer), but protocol drivers set *file = bs (ie. 
themselves).  As long as you read it as "the offset is valid in the 
returned *file", and are careful as to _which_ BDS gets returned in 
*file*, it can still make sense.

So next I tried playing with a patch, to see how much returning 
OFFSET_VALID with DATA matters; and it turns out is is easily observable 
anywhere that the underlying protocol bleeds through to the format layer 
(particularly the raw format driver):

$ echo abc > tmp
$ truncate --size=10M tmp

pre-patch:
$ ./qemu-img map --output=json tmp
[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true, 
"offset": 0},
{ "start": 4096, "length": 10481664, "depth": 0, "zero": true, "data": 
false, "offset": 4096}]

turn off OFFSET_VALID at the protocol layer:
diff --git i/block/file-posix.c w/block/file-posix.c
index f1591c38490..c05992c1121 100644
--- i/block/file-posix.c
+++ w/block/file-posix.c
@@ -2158,9 +2158,7 @@ static int coroutine_fn 
raw_co_block_status(BlockDriverState *bs,

     if (!want_zero) {
         *pnum = bytes;
-        *map = offset;
-        *file = bs;
-        return BDRV_BLOCK_DATA | BDRV_BLOCK_OFFSET_VALID;
+        return BDRV_BLOCK_DATA;
     }

     ret = find_allocation(bs, offset, &data, &hole);
@@ -2183,9 +2181,7 @@ static int coroutine_fn 
raw_co_block_status(BlockDriverState *bs,
         *pnum = MIN(bytes, data - offset);
         ret = BDRV_BLOCK_ZERO;
     }
-    *map = offset;
-    *file = bs;
-    return ret | BDRV_BLOCK_OFFSET_VALID;
+    return ret;
 }

 static coroutine_fn BlockAIOCB *raw_aio_pdiscard(BlockDriverState *bs,


post-patch:
$ ./qemu-img map --output=json tmp
[{ "start": 0, "length": 4096, "depth": 0, "zero": false, "data": true},
{ "start": 4096, "length": 10481664, "depth": 0, "zero": true, "data": 
false}]


> OFFSET_VALID | DATA might be excusable because I can see that it's
> convenient that a protocol driver refers to itself as *file instead of
> returning NULL there and then the offset is valid (though it would be
> pointless to actually follow the file pointer), but OFFSET_VALID without
> DATA probably isn't.

So OFFSET_VALID | DATA for a protocol BDS is not just convenient, but 
necessary to avoid breaking qemu-img map output.  But you are also right 
that OFFSET_VALID without data makes little sense at a protocol layer. 
So with that in mind, I'm auditing all of the protocol layers to make 
sure OFFSET_VALID ends up as something sane.

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org