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

[Max Reitz <mreitz@redhat.com>]

On 10.03.21 17:35, Fam Zheng wrote:
> 
> 
> On Wed, 10 Mar 2021 at 15:02, Max Reitz <mreitz@redhat.com 
> <mailto:mreitz@redhat.com>> wrote:
> 
>     On 10.03.21 15:17, fam@euphon.net <mailto:fam@euphon.net> wrote:
>      > From: Fam Zheng <famzheng@amazon.com <mailto: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 <mailto: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.
> 
> 
> But using null-co:// in tests is not wrong, the problem is the caller 
> failed to initialize its buffer.

Then I don’t see why we’d need zero-co://.

Max