Re: [PATCH 2/2] block/null: Add read-pattern option

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> On Mon, Apr 28, 2025 at 01:59:00AM +0300, Nir Soffer wrote:
>> When the `read-zeroes` is set, reads produce zeroes, and block status
>> return BDRV_BLOCK_ZERO, emulating a sparse image.
>> 
>> If we don't set `read-zeros` we report BDRV_BLOCK_DATA, but image data
>> is undefined; posix_memalign, _aligned_malloc, valloc, or memalign do
>> not promise to zero allocated memory.
>> 
>> When computing a blkhash of an image via qemu-nbd, we want to test 3
>> cases:
>> 
>> 1. Sparse image: skip reading the entire image based on block status
>>    result, and use a pre-computed zero block hash.
>> 2. Image full of zeroes: read the entire image, detect block full of
>>    zeroes and skip block hash computation.
>> 3. Image full of data: read the entire image and compute a hash of all
>>    blocks.
>> 
>> This change adds `read-pattern` option. If the option is set, reads
>> produce the specified pattern. With this option we can emulate an image
>> full of zeroes or full of non-zeroes.
>> 
>> Specifying both `read-zeroes` and `read-pattern != 0` is not useful
>> since `read-zeroes` implies a sparse image.  In this case `read-zeroes`
>> wins and we ignore the pattern. Maybe we need to make the options mutual
>> exclusive.
>
> I would lean towards an error.  It's easier to remove an error later
> if we find it was too strict, than to be lax up front and then regret
> it down the road when we wish we had been more strict.

Seconded.  Silently "fixing" the user's nonsensical instructions is
commonly a bad idea.

>> The following examples shows how the new option can be used with blksum
>> (or nbdcopy --blkhash) to compute a blkhash of an image using the
>> null-co driver.
>> 
>> Sparse image - the very fast path:
>> 
>>     % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/sparse.sock \
>>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-zeroes': true}}" &
>> 
>>     % time blksum 'nbd+unix:///?socket=/tmp/sparse.sock'
>>     300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/sparse.sock
>>     blksum 'nbd+unix:///?socket=/tmp/sparse.sock'  0.05s user 0.01s system 92% cpu 0.061 total
>> 
>> Image full of zeros - same hash, 268 times slower:
>> 
>>     % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/zero.sock \
>>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': 0}}" &
>> 
>>     % time blksum 'nbd+unix:///?socket=/tmp/zero.sock'
>>     300ad1efddb063822fea65ae3174cd35320939d4d0b050613628c6e1e876f8f6  nbd+unix:///?socket=/tmp/zero.sock
>>     blksum 'nbd+unix:///?socket=/tmp/zero.sock'  7.45s user 22.57s system 183% cpu 16.347 total
>> 
>> Image full of data - difference hash, heavy cpu usage:
>> 
>>     % ./qemu-nbd -r -t -e 0 -f raw -k /tmp/data.sock \
>>         "json:{'driver': 'raw', 'file': {'driver': 'null-co', 'size': '100g', 'read-pattern': -1}}" &
>> 
>>     % time blksum 'nbd+unix:///?socket=/tmp/data.sock'
>>     2c122b3ed28c83ede3c08485659fa9b56ee54ba1751db74d8ba9aa13d9866432  nbd+unix:///?socket=/tmp/data.sock
>>     blksum 'nbd+unix:///?socket=/tmp/data.sock'  46.05s user 14.15s system 448% cpu 13.414 total
>> 
>> Tested on top of
>> https://lists.gnu.org/archive/html/qemu-devel/2025-04/msg05096.html.
>> 
>> Signed-off-by: Nir Soffer <nirsof@gmail.com>
>> ---
>>  block/null.c         | 17 +++++++++++++++++
>>  qapi/block-core.json |  9 ++++++++-
>>  2 files changed, 25 insertions(+), 1 deletion(-)
>
> Should block status return ZERO|DATA when read-pattern=0 is present?
>
   diff --git a/qapi/block-core.json b/qapi/block-core.json
   index b1937780e1..7d576cccbb 100644
   --- a/qapi/block-core.json
   +++ b/qapi/block-core.json
   @@ -3297,10 +3297,17 @@
    # @read-zeroes: if true, reads from the device produce zeroes; if
    #     false, the buffer is left unchanged.
    #     (default: false; since: 4.1)

The commit message explains "read-zeroes": true behaves like a sparse
image.  The existing doc comment does not.  I suspect it should.

    #
>> +# @read-pattern: if set, reads from the device produce the specified
>> +#     pattern; if unset, the buffer is left unchanged.
>> +#     (since: 10.1)
>> +#
>>  # Since: 2.9
>>  ##
>>  { 'struct': 'BlockdevOptionsNull',
>> -  'data': { '*size': 'int', '*latency-ns': 'uint64', '*read-zeroes': 'bool' } }
>> +  'data': { '*size': 'int',
>> +            '*latency-ns': 'uint64',
>> +            '*read-zeroes': 'bool',
>> +            '*read-pattern': 'int' } }
>
> Should this be 'uint8' instead of 'int', so that we aren't silently
> truncating spurious upper bits when passing it to memset()?

Yes, please.

Without this, the doc comment does not sufficiently specify the contents
of the image.  "The specified pattern" could be read as

* Four bytes given by the 32 bit value of @read-pattern in big endian

* or in little endian

* or in host endian

In fact, it's none of the above, it's the least significant byte.

Please try to clarify the doc comment in addition to narrowing the type.