| Message ID | 20210310141752.5113-1-fam@euphon.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: Introduce zero-co:// and zero-aio:// | expand |
On 3/10/21 3:17 PM, 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://. Thanks! > > Signed-off-by: Fam Zheng <fam@euphon.net> > --- > block/null.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 91 insertions(+) > +static int zero_file_open(BlockDriverState *bs, QDict *options, int flags, > + Error **errp) > +{ > + QemuOpts *opts; > + BDRVNullState *s = bs->opaque; > + int ret = 0; > + > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &error_abort); > + s->length = > + qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30); Please do not use this magic default value, let's always explicit the size. s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0); if (s->length < 0) { error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE); ret = -EINVAL; } > + s->latency_ns = > + qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0); > + if (s->latency_ns < 0) { > + error_setg(errp, "latency-ns is invalid"); > + ret = -EINVAL; > + } > + s->read_zeroes = true; > + qemu_opts_del(opts); > + bs->supported_write_flags = BDRV_REQ_FUA; > + return ret; > +} Otherwise LGTM :)
On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 3/10/21 3:17 PM, 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://. > > Thanks! > > > > > Signed-off-by: Fam Zheng <fam@euphon.net> > > --- > > block/null.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > > +static int zero_file_open(BlockDriverState *bs, QDict *options, int > flags, > > + Error **errp) > > +{ > > + QemuOpts *opts; > > + BDRVNullState *s = bs->opaque; > > + int ret = 0; > > + > > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > > + qemu_opts_absorb_qdict(opts, options, &error_abort); > > + s->length = > > + qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30); > > Please do not use this magic default value, let's always explicit the > size. > > s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0); > if (s->length < 0) { > error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE); > ret = -EINVAL; > } Doesn't that result in a bare -blockdev zero-co:// would have 0 byte in size? I'm copying what null-co:// does today, and it's convenient for tests. Why is a default size bad? Fam > > > + s->latency_ns = > > + qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0); > > + if (s->latency_ns < 0) { > > + error_setg(errp, "latency-ns is invalid"); > > + ret = -EINVAL; > > + } > > + s->read_zeroes = true; > > + qemu_opts_del(opts); > > + bs->supported_write_flags = BDRV_REQ_FUA; > > + return ret; > > +} > > Otherwise LGTM :) > > >
10.03.2021 17: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(+) > > diff --git a/block/null.c b/block/null.c > index cc9b1d4ea7..5de97e8fda 100644 > --- a/block/null.c > +++ b/block/null.c > @@ -76,6 +76,30 @@ static void null_aio_parse_filename(const char *filename, QDict *options, > } > } > > +static void zero_co_parse_filename(const char *filename, QDict *options, > + Error **errp) > +{ > + /* This functions only exists so that a zero-co:// filename is accepted > + * with the zero-co driver. */ > + if (strcmp(filename, "zero-co://")) { > + error_setg(errp, "The only allowed filename for this driver is " > + "'zero-co://'"); > + return; > + } > +} > + > +static void zero_aio_parse_filename(const char *filename, QDict *options, > + Error **errp) > +{ > + /* This functions only exists so that a zero-aio:// filename is accepted > + * with the zero-aio driver. */ > + if (strcmp(filename, "zero-aio://")) { > + error_setg(errp, "The only allowed filename for this driver is " > + "'zero-aio://'"); > + return; > + } > +} > + > static int null_file_open(BlockDriverState *bs, QDict *options, int flags, > Error **errp) > { > @@ -99,6 +123,29 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags, > return ret; > } > > +static int zero_file_open(BlockDriverState *bs, QDict *options, int flags, > + Error **errp) > +{ > + QemuOpts *opts; > + BDRVNullState *s = bs->opaque; > + int ret = 0; > + > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > + qemu_opts_absorb_qdict(opts, options, &error_abort); > + s->length = > + qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30); > + s->latency_ns = > + qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0); > + if (s->latency_ns < 0) { > + error_setg(errp, "latency-ns is invalid"); > + ret = -EINVAL; > + } > + s->read_zeroes = true; > + qemu_opts_del(opts); > + bs->supported_write_flags = BDRV_REQ_FUA; > + return ret; > +} > + > static int64_t null_getlength(BlockDriverState *bs) > { > BDRVNullState *s = bs->opaque; > @@ -316,10 +363,54 @@ static BlockDriver bdrv_null_aio = { > .strong_runtime_opts = null_strong_runtime_opts, > }; > > +static BlockDriver bdrv_zero_co = { > + .format_name = "zero-co", > + .protocol_name = "zero-co", > + .instance_size = sizeof(BDRVNullState), > + > + .bdrv_file_open = zero_file_open, > + .bdrv_parse_filename = zero_co_parse_filename, > + .bdrv_getlength = null_getlength, > + .bdrv_get_allocated_file_size = null_allocated_file_size, > + > + .bdrv_co_preadv = null_co_preadv, > + .bdrv_co_pwritev = null_co_pwritev, > + .bdrv_co_flush_to_disk = null_co_flush, > + .bdrv_reopen_prepare = null_reopen_prepare, > + > + .bdrv_co_block_status = null_co_block_status, > + > + .bdrv_refresh_filename = null_refresh_filename, > + .strong_runtime_opts = null_strong_runtime_opts, > +}; > + > +static BlockDriver bdrv_zero_aio = { > + .format_name = "zero-aio", > + .protocol_name = "zero-aio", > + .instance_size = sizeof(BDRVNullState), > + > + .bdrv_file_open = zero_file_open, > + .bdrv_parse_filename = zero_aio_parse_filename, > + .bdrv_getlength = null_getlength, > + .bdrv_get_allocated_file_size = null_allocated_file_size, > + > + .bdrv_aio_preadv = null_aio_preadv, > + .bdrv_aio_pwritev = null_aio_pwritev, > + .bdrv_aio_flush = null_aio_flush, > + .bdrv_reopen_prepare = null_reopen_prepare, > + > + .bdrv_co_block_status = null_co_block_status, > + > + .bdrv_refresh_filename = null_refresh_filename, > + .strong_runtime_opts = null_strong_runtime_opts, > +}; I don't think we need separate zero-aio driver. What is the actual difference? Probably zero-co is enough, and we can call it just zero. And then, maybe add null driver (same as null-co, but without read_zeroes option) and deprecate null-co and null-aio drivers. Thus we'll get two clean well defined things: null and zero drivers.. > + > static void bdrv_null_init(void) > { > bdrv_register(&bdrv_null_co); > bdrv_register(&bdrv_null_aio); > + bdrv_register(&bdrv_zero_co); > + bdrv_register(&bdrv_zero_aio); > } > > block_init(bdrv_null_init); >
On 3/10/21 3:28 PM, Fam Zheng wrote: > On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé <philmd@redhat.com > <mailto:philmd@redhat.com>> wrote: > > On 3/10/21 3:17 PM, 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://. > > Thanks! > > > > > Signed-off-by: Fam Zheng <fam@euphon.net <mailto:fam@euphon.net>> > > --- > > block/null.c | 91 > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 91 insertions(+) > > > +static int zero_file_open(BlockDriverState *bs, QDict *options, > int flags, > > + Error **errp) > > +{ > > + QemuOpts *opts; > > + BDRVNullState *s = bs->opaque; > > + int ret = 0; > > + > > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > > + qemu_opts_absorb_qdict(opts, options, &error_abort); > > + s->length = > > + qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30); > > Please do not use this magic default value, let's always explicit the > size. > > s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0); > if (s->length < 0) { > error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE); > ret = -EINVAL; > } > > > Doesn't that result in a bare > > -blockdev zero-co:// > > would have 0 byte in size? Yes, this will display an error. Maybe better something like: if (s->length == 0) { error_append_hint(errp, "Usage: zero-co://,size=NUM"); error_setg(errp, "size must be specified"); ret = -EINVAL; } else if (s->length < 0) { error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE); ret = -EINVAL; } > > I'm copying what null-co:// does today, and it's convenient for tests. > Why is a default size bad? We learned default are almost always bad because you can not get rid of them. Also because we have reports of errors when using floppy and another one (can't remember) with null-co because of invalid size and we have to explain "the default is 1GB, you have to explicit your size". Phil.
On Wed, 10 Mar 2021 at 14:51, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > On 3/10/21 3:28 PM, Fam Zheng wrote: > > On Wed, 10 Mar 2021 at 14:24, Philippe Mathieu-Daudé <philmd@redhat.com > > <mailto:philmd@redhat.com>> wrote: > > > > On 3/10/21 3:17 PM, 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://. > > > > Thanks! > > > > > > > > Signed-off-by: Fam Zheng <fam@euphon.net <mailto:fam@euphon.net>> > > > --- > > > block/null.c | 91 > > ++++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 91 insertions(+) > > > > > +static int zero_file_open(BlockDriverState *bs, QDict *options, > > int flags, > > > + Error **errp) > > > +{ > > > + QemuOpts *opts; > > > + BDRVNullState *s = bs->opaque; > > > + int ret = 0; > > > + > > > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > > > + qemu_opts_absorb_qdict(opts, options, &error_abort); > > > + s->length = > > > + qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30); > > > > Please do not use this magic default value, let's always explicit the > > size. > > > > s->length = qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 0); > > if (s->length < 0) { > > error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE); > > ret = -EINVAL; > > } > > > > > > Doesn't that result in a bare > > > > -blockdev zero-co:// > > > > would have 0 byte in size? > > Yes, this will display an error. > > Maybe better something like: > > if (s->length == 0) { > error_append_hint(errp, "Usage: zero-co://,size=NUM"); > error_setg(errp, "size must be specified"); > ret = -EINVAL; > } else if (s->length < 0) { > error_setg(errp, "%s is invalid", BLOCK_OPT_SIZE); > ret = -EINVAL; > } > > > > > I'm copying what null-co:// does today, and it's convenient for tests. > > Why is a default size bad? > > We learned default are almost always bad because you can not get > rid of them. Also because we have reports of errors when using > floppy and another one (can't remember) with null-co because of > invalid size and we have to explain "the default is 1GB, you have > to explicit your size". > Yeah I think that makes sense. I'll revise. Thanks, Fam
On Wed, 10 Mar 2021 at 14:41, Vladimir Sementsov-Ogievskiy < vsementsov@virtuozzo.com> wrote: > 10.03.2021 17: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(+) > > > > diff --git a/block/null.c b/block/null.c > > index cc9b1d4ea7..5de97e8fda 100644 > > --- a/block/null.c > > +++ b/block/null.c > > @@ -76,6 +76,30 @@ static void null_aio_parse_filename(const char > *filename, QDict *options, > > } > > } > > > > +static void zero_co_parse_filename(const char *filename, QDict *options, > > + Error **errp) > > +{ > > + /* This functions only exists so that a zero-co:// filename is > accepted > > + * with the zero-co driver. */ > > + if (strcmp(filename, "zero-co://")) { > > + error_setg(errp, "The only allowed filename for this driver is " > > + "'zero-co://'"); > > + return; > > + } > > +} > > + > > +static void zero_aio_parse_filename(const char *filename, QDict > *options, > > + Error **errp) > > +{ > > + /* This functions only exists so that a zero-aio:// filename is > accepted > > + * with the zero-aio driver. */ > > + if (strcmp(filename, "zero-aio://")) { > > + error_setg(errp, "The only allowed filename for this driver is " > > + "'zero-aio://'"); > > + return; > > + } > > +} > > + > > static int null_file_open(BlockDriverState *bs, QDict *options, int > flags, > > Error **errp) > > { > > @@ -99,6 +123,29 @@ static int null_file_open(BlockDriverState *bs, > QDict *options, int flags, > > return ret; > > } > > > > +static int zero_file_open(BlockDriverState *bs, QDict *options, int > flags, > > + Error **errp) > > +{ > > + QemuOpts *opts; > > + BDRVNullState *s = bs->opaque; > > + int ret = 0; > > + > > + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); > > + qemu_opts_absorb_qdict(opts, options, &error_abort); > > + s->length = > > + qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30); > > + s->latency_ns = > > + qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0); > > + if (s->latency_ns < 0) { > > + error_setg(errp, "latency-ns is invalid"); > > + ret = -EINVAL; > > + } > > + s->read_zeroes = true; > > + qemu_opts_del(opts); > > + bs->supported_write_flags = BDRV_REQ_FUA; > > + return ret; > > +} > > + > > static int64_t null_getlength(BlockDriverState *bs) > > { > > BDRVNullState *s = bs->opaque; > > @@ -316,10 +363,54 @@ static BlockDriver bdrv_null_aio = { > > .strong_runtime_opts = null_strong_runtime_opts, > > }; > > > > +static BlockDriver bdrv_zero_co = { > > + .format_name = "zero-co", > > + .protocol_name = "zero-co", > > + .instance_size = sizeof(BDRVNullState), > > + > > + .bdrv_file_open = zero_file_open, > > + .bdrv_parse_filename = zero_co_parse_filename, > > + .bdrv_getlength = null_getlength, > > + .bdrv_get_allocated_file_size = null_allocated_file_size, > > + > > + .bdrv_co_preadv = null_co_preadv, > > + .bdrv_co_pwritev = null_co_pwritev, > > + .bdrv_co_flush_to_disk = null_co_flush, > > + .bdrv_reopen_prepare = null_reopen_prepare, > > + > > + .bdrv_co_block_status = null_co_block_status, > > + > > + .bdrv_refresh_filename = null_refresh_filename, > > + .strong_runtime_opts = null_strong_runtime_opts, > > +}; > > + > > +static BlockDriver bdrv_zero_aio = { > > + .format_name = "zero-aio", > > + .protocol_name = "zero-aio", > > + .instance_size = sizeof(BDRVNullState), > > + > > + .bdrv_file_open = zero_file_open, > > + .bdrv_parse_filename = zero_aio_parse_filename, > > + .bdrv_getlength = null_getlength, > > + .bdrv_get_allocated_file_size = null_allocated_file_size, > > + > > + .bdrv_aio_preadv = null_aio_preadv, > > + .bdrv_aio_pwritev = null_aio_pwritev, > > + .bdrv_aio_flush = null_aio_flush, > > + .bdrv_reopen_prepare = null_reopen_prepare, > > + > > + .bdrv_co_block_status = null_co_block_status, > > + > > + .bdrv_refresh_filename = null_refresh_filename, > > + .strong_runtime_opts = null_strong_runtime_opts, > > +}; > > I don't think we need separate zero-aio driver. What is the actual > difference? > > Probably zero-co is enough, and we can call it just zero. And then, maybe > add null driver (same as null-co, but without read_zeroes option) and > deprecate null-co and null-aio drivers. Thus we'll get two clean well > defined things: null and zero drivers.. > Sounds good to me, I'll just call it zero:// for this patch and leave the null convergence and deprecation business for another patch. Fam > > > + > > static void bdrv_null_init(void) > > { > > bdrv_register(&bdrv_null_co); > > bdrv_register(&bdrv_null_aio); > > + bdrv_register(&bdrv_zero_co); > > + bdrv_register(&bdrv_zero_aio); > > } > > > > block_init(bdrv_null_init); > > > > > -- > Best regards, > Vladimir > >
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
On Wed, 10 Mar 2021 at 15:02, Max Reitz <mreitz@redhat.com> wrote: > 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. > But using null-co:// in tests is not wrong, the problem is the caller failed to initialize its buffer. IMO the valgrind issue should really be fixed like this: diff --git a/hw/block/hd-geometry.c b/hw/block/hd-geometry.c index dcbccee294..9078718445 100644 --- a/hw/block/hd-geometry.c +++ b/hw/block/hd-geometry.c @@ -55,7 +55,7 @@ struct partition { static int guess_disk_lchs(BlockBackend *blk, int *pcylinders, int *pheads, int *psectors) { - uint8_t buf[BDRV_SECTOR_SIZE]; + uint8_t buf[BDRV_SECTOR_SIZE] = {}; int i, heads, sectors, cylinders; struct partition *p; uint32_t nr_sects;
Am 10.03.2021 um 15:17 hat fam@euphon.net geschrieben: > 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 ++++++++++++++++++++++++++++++++++++++++++++++++++++ If we're adding new block drivers, there should certainly be some QAPI schema updates here. Kevin
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
diff --git a/block/null.c b/block/null.c index cc9b1d4ea7..5de97e8fda 100644 --- a/block/null.c +++ b/block/null.c @@ -76,6 +76,30 @@ static void null_aio_parse_filename(const char *filename, QDict *options, } } +static void zero_co_parse_filename(const char *filename, QDict *options, + Error **errp) +{ + /* This functions only exists so that a zero-co:// filename is accepted + * with the zero-co driver. */ + if (strcmp(filename, "zero-co://")) { + error_setg(errp, "The only allowed filename for this driver is " + "'zero-co://'"); + return; + } +} + +static void zero_aio_parse_filename(const char *filename, QDict *options, + Error **errp) +{ + /* This functions only exists so that a zero-aio:// filename is accepted + * with the zero-aio driver. */ + if (strcmp(filename, "zero-aio://")) { + error_setg(errp, "The only allowed filename for this driver is " + "'zero-aio://'"); + return; + } +} + static int null_file_open(BlockDriverState *bs, QDict *options, int flags, Error **errp) { @@ -99,6 +123,29 @@ static int null_file_open(BlockDriverState *bs, QDict *options, int flags, return ret; } +static int zero_file_open(BlockDriverState *bs, QDict *options, int flags, + Error **errp) +{ + QemuOpts *opts; + BDRVNullState *s = bs->opaque; + int ret = 0; + + opts = qemu_opts_create(&runtime_opts, NULL, 0, &error_abort); + qemu_opts_absorb_qdict(opts, options, &error_abort); + s->length = + qemu_opt_get_size(opts, BLOCK_OPT_SIZE, 1 << 30); + s->latency_ns = + qemu_opt_get_number(opts, NULL_OPT_LATENCY, 0); + if (s->latency_ns < 0) { + error_setg(errp, "latency-ns is invalid"); + ret = -EINVAL; + } + s->read_zeroes = true; + qemu_opts_del(opts); + bs->supported_write_flags = BDRV_REQ_FUA; + return ret; +} + static int64_t null_getlength(BlockDriverState *bs) { BDRVNullState *s = bs->opaque; @@ -316,10 +363,54 @@ static BlockDriver bdrv_null_aio = { .strong_runtime_opts = null_strong_runtime_opts, }; +static BlockDriver bdrv_zero_co = { + .format_name = "zero-co", + .protocol_name = "zero-co", + .instance_size = sizeof(BDRVNullState), + + .bdrv_file_open = zero_file_open, + .bdrv_parse_filename = zero_co_parse_filename, + .bdrv_getlength = null_getlength, + .bdrv_get_allocated_file_size = null_allocated_file_size, + + .bdrv_co_preadv = null_co_preadv, + .bdrv_co_pwritev = null_co_pwritev, + .bdrv_co_flush_to_disk = null_co_flush, + .bdrv_reopen_prepare = null_reopen_prepare, + + .bdrv_co_block_status = null_co_block_status, + + .bdrv_refresh_filename = null_refresh_filename, + .strong_runtime_opts = null_strong_runtime_opts, +}; + +static BlockDriver bdrv_zero_aio = { + .format_name = "zero-aio", + .protocol_name = "zero-aio", + .instance_size = sizeof(BDRVNullState), + + .bdrv_file_open = zero_file_open, + .bdrv_parse_filename = zero_aio_parse_filename, + .bdrv_getlength = null_getlength, + .bdrv_get_allocated_file_size = null_allocated_file_size, + + .bdrv_aio_preadv = null_aio_preadv, + .bdrv_aio_pwritev = null_aio_pwritev, + .bdrv_aio_flush = null_aio_flush, + .bdrv_reopen_prepare = null_reopen_prepare, + + .bdrv_co_block_status = null_co_block_status, + + .bdrv_refresh_filename = null_refresh_filename, + .strong_runtime_opts = null_strong_runtime_opts, +}; + static void bdrv_null_init(void) { bdrv_register(&bdrv_null_co); bdrv_register(&bdrv_null_aio); + bdrv_register(&bdrv_zero_co); + bdrv_register(&bdrv_zero_aio); } block_init(bdrv_null_init);