block/null: Add read-pattern

[PATCH 1/2] block/null: Report DATA if not reading zeroes

Posted by Nir Soffer 6 months, 3 weeks ago

If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
confuse users or other programs:

    % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
    [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]

    % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &

    % nbdinfo --map nbd://127.0.0.1
             0  1073741824    1  hole

With this change we report DATA in this case:

    % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
    [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]

    % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &

    % nbdinfo --map nbd://127.0.0.1
             0  1073741824    0  data

Signed-off-by: Nir Soffer <nirsof@gmail.com>
---
 block/null.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/block/null.c b/block/null.c
index dc0b1fdbd9..7ba87bd9a9 100644
--- a/block/null.c
+++ b/block/null.c
@@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
     *map = offset;
     *file = bs;
 
-    if (s->read_zeroes) {
-        ret |= BDRV_BLOCK_ZERO;
-    }
+    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
     return ret;
 }
 
-- 
2.39.5 (Apple Git-154)

Re: [PATCH 1/2] block/null: Report DATA if not reading zeroes

Posted by Eric Blake 6 months, 3 weeks ago

On Mon, Apr 28, 2025 at 01:58:59AM +0300, Nir Soffer wrote:
> If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
> BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
> confuse users or other programs:
> 
>     % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>     [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]
> 
>     % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
> 
>     % nbdinfo --map nbd://127.0.0.1
>              0  1073741824    1  hole
> 
> With this change we report DATA in this case:
> 
>     % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>     [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]

Do we really want to represent the region as present?  I think we're
okay (we guarantee that reads from the region will return sensible
contents), and your next patch to allow control over what data is seen
makes it even better.

> 
>     % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
> 
>     % nbdinfo --map nbd://127.0.0.1
>              0  1073741824    0  data
> 
> Signed-off-by: Nir Soffer <nirsof@gmail.com>
> ---
>  block/null.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/block/null.c b/block/null.c
> index dc0b1fdbd9..7ba87bd9a9 100644
> --- a/block/null.c
> +++ b/block/null.c
> @@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
>      *map = offset;
>      *file = bs;
>  
> -    if (s->read_zeroes) {
> -        ret |= BDRV_BLOCK_ZERO;
> -    }
> +    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
>      return ret;

According to include/block/block-common.h,

 * BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer
 * BDRV_BLOCK_ZERO: offset reads as zero
 * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data

Or read differently, BDRV_BLOCK_DATA says an area is not sparse.  This
patch is changing the null driver to report the entire image as either
sparse and zero, or not sparse and not zero.

For testing purposes, I think that is reasonable enough; it matches
that file-posix.c always returns one or the other, when using only
lseek() to probe things.  NBD has a bit finer-grained control (it has
two bits, one for sparseness which is effectively !BDRV_BLOCK_DATA,
and one for zero; where you can have something that is sparse but does
not read as zero, such as SCSI that lacks coherent uninitialized
read).  In fact, you could even have DATA|ZERO if we know something is
allocated AND that it reads as zero (which will be the case in your
next patch if the user explicitly asks for a read pattern of 0).

But for all of that thinking, in the end your patch makes sense to me.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.
Virtualization:  qemu.org | libguestfs.org

Re: [PATCH 1/2] block/null: Report DATA if not reading zeroes

Posted by Nir Soffer 6 months, 2 weeks ago


> On 29 Apr 2025, at 0:49, Eric Blake <eblake@redhat.com> wrote:
> 
> On Mon, Apr 28, 2025 at 01:58:59AM +0300, Nir Soffer wrote:
>> If read-zeroes is not set, we did not report BDRV_BLOCK_DATA or
>> BDRV_BLOCK_ZERO. This is not consistent with other drivers and can
>> confuse users or other programs:
>> 
>>    % qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>    [{ "start": 0, "length": 1073741824, "depth": 0, "present": false, "zero": false, "data": false, "compressed": false}]
>> 
>>    % qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>> 
>>    % nbdinfo --map nbd://127.0.0.1
>>             0  1073741824    1  hole
>> 
>> With this change we report DATA in this case:
>> 
>>    % ./qemu-img map --output json "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}"
>>    [{ "start": 0, "length": 1073741824, "depth": 0, "present": true, "zero": false, "data": true, "compressed": false, "offset": 0}]
> 
> Do we really want to represent the region as present?  I think we're
> okay (we guarantee that reads from the region will return sensible
> contents), and your next patch to allow control over what data is seen
> makes it even better.

I kept the existing behavior, but I think present=false is relevant only if you have a backing file (like qcow2), so for null-co it makes sense to behave like file driver.

> 
> 
>> 
>>    % ./qemu-nbd "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '1g'}}" &
>> 
>>    % nbdinfo --map nbd://127.0.0.1
>>             0  1073741824    0  data
>> 
>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
>> ---
>> block/null.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/block/null.c b/block/null.c
>> index dc0b1fdbd9..7ba87bd9a9 100644
>> --- a/block/null.c
>> +++ b/block/null.c
>> @@ -239,9 +239,7 @@ static int coroutine_fn null_co_block_status(BlockDriverState *bs,
>>     *map = offset;
>>     *file = bs;
>> 
>> -    if (s->read_zeroes) {
>> -        ret |= BDRV_BLOCK_ZERO;
>> -    }
>> +    ret |= s->read_zeroes ? BDRV_BLOCK_ZERO : BDRV_BLOCK_DATA;
>>     return ret;
> 
> According to include/block/block-common.h,
> 
> * BDRV_BLOCK_DATA: allocation for data at offset is tied to this layer
> * BDRV_BLOCK_ZERO: offset reads as zero
> * BDRV_BLOCK_OFFSET_VALID: an associated offset exists for accessing raw data
> 
> Or read differently, BDRV_BLOCK_DATA says an area is not sparse.  This
> patch is changing the null driver to report the entire image as either
> sparse and zero, or not sparse and not zero.
> 
> For testing purposes, I think that is reasonable enough; it matches
> that file-posix.c always returns one or the other, when using only
> lseek() to probe things.  NBD has a bit finer-grained control (it has
> two bits, one for sparseness which is effectively !BDRV_BLOCK_DATA,
> and one for zero; where you can have something that is sparse but does
> not read as zero, such as SCSI that lacks coherent uninitialized
> read).  In fact, you could even have DATA|ZERO if we know something is
> allocated AND that it reads as zero (which will be the case in your
> next patch if the user explicitly asks for a read pattern of 0).

If we want this we can remove read_zeros, since read_pattern support reading zeros, and have:

    *read_pattern: uint8 (default unset)
    *block_status: “data"|”zero” (default data?)
    *allocated: bool (default true)

We can replace reads-zeros=on with read-pattern=0 in tests and docs, but it may break other users using null-co.

Maybe keep read_zeros for backward compatibility, and make it conflict with the new options?

> 
> But for all of that thinking, in the end your patch makes sense to me.
> 
> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.
> Virtualization:  qemu.org | libguestfs.org