qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Max Reitz
Subject: Re: [PATCH] block: Introduce zero-co:// and zero-aio://
Date: Thu, 11 Mar 2021 13:11:23 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]