Re: [PATCH] block: Introduce zero-co:// and zero-aio://

[Max Reitz]

On 10.03.21 15:17, fam@euphon.net wrote:
> From: Fam Zheng <famzheng@amazon.com>
>
> null-co:// has a read-zeroes=off default, when used to in security
> analysis, this can cause false positives because the driver doesn't
> write to the read buffer.
>
> null-co:// has the highest possible performance as a block driver, so
> let's keep it that way. This patch introduces zero-co:// and
> zero-aio://, largely similar with null-*://, but have read-zeroes=on by
> default, so it's more suitable in cases than null-co://.
>
> Signed-off-by: Fam Zheng <fam@euphon.net>
> ---
>   block/null.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 91 insertions(+)

You’ll also need to make all tests that currently use null-{co,aio} use 
zero-{co,aio}, because none of those are performance tests (as far as 
I’m aware), so they all want a block driver that memset()s.

(And that’s basically my problem with this approach; nearly everyone who 
uses null-* right now probably wants read-zeroes=on, so keeping null-* 
as it is means all of those users should be changed.  Sure, they were 
all wrong to not specify read-zeroes=on, but that’s how it is.  So while 
technically this patch is a compatible change, in contrast to the one 
making read-zeroes=on the default, in practice it absolutely isn’t.)

Another problem arising from that is I can imagine that some 
distributions might have whitelisted null-co because many iotests 
implicitly depend on it, so the iotests will fail if they aren’t 
whitelisted.  Now they’d need to whitelist zero-co, too.  Not 
impossible, sure, but it’s work that would need to be done.


My problem is this: We have a not-really problem, namely “valgrind and 
other tools complain”.  Philippe (and I guess me on IRC) proposed a 
simple solution whose only drawbacks (AFAIU) are:

(1) When writing performance tests, you’ll then need to explicitly 
specify read-zeroes=off.  Existing performance tests using null-co will 
probably wrongly show degredation.  (Are there such tests, though?)

(2) null will not quite conform to its name, strictly speaking. 
Frankly, I don’t know who’d care other than people who write those 
performance tests mentioned in (1).  I know I don’t care.

(Technically: (3) We should look into all qemu tests that use null-co to 
see whether they test performance.  In practice, they don’t, so we don’t 
need to.)

So AFAIU change the read-zeroes default would affect very few people, if 
any.  I see you care about (2), and you’re the maintainer, so I can’t 
say that there is no problem.  But it isn’t a practical one.

So on the practical front, we get a small benefit (tools won’t complain) 
for a small drawback (having to remember read-zeroes=off for performance 
tests).


Now you propose a change that has the following drawbacks, as I see it:

(1) All non-performance tests using null-* should be changed to zero-*. 
 And those are quite a few tests, so this is some work.

(2) Distributions that have whitelisted null-co now should whitelist 
zero-co, too.

Not impossible, but I consider these much bigger practical drawbacks 
than making read-zeroes=on the default.  It’s actual work that must be 
done.  To me personally, these drawbacks far outweigh the benefit of 
having valgrind and other tools not complain.


I can’t stop you from updating this patch to do (1), but it doesn’t make 
sense to me from a practical perspective.  It just doesn’t seem worth it 
to me.

Max