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:

  *  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

$ ./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;

     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,

$ ./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

Reply via email to