| Message ID | 1501597152-25342-3-git-send-email-anton.nefedov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 08/01/2017 09:18 AM, Anton Nefedov wrote: > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > --- > block/blkverify.c | 9 +++++++++ > 1 file changed, 9 insertions(+) Basically, blkverify supports a flag if BOTH of its underlying files also support the flag; if either side can't handle the flag, then we fall back to emulation for both sides. With more overhead, we COULD state that we support both bits if at least one of the two underlying BDS supports the bit, and then emulate support for the bit on the second BDS where it was lacking, so that at least the first BDS doesn't suffer from the penalties of the fallbacks. But that means duplicating the block layer fallback code in blkverify, which is already something that we don't necessarily expect high performance from. For FUA, failure to implement the bit merely means that we have more device-wide flush calls (instead of per-transaction mini-flushes), but the end data should be the same. But for MAY_UNMAP, I'm worried that we may have situations where a plain BDS will create holes, while running the same device paired through blkverify will fall back to slower explicit zeroes. I'm wondering whether this will bite us, if we have scenarios where the mere fact of trying to verify block device behavior changes what behavior we are even verifying. Thus, while I think the code change _looks_ okay, I'm not sure if it is correct design-wise, nor whether it is 2.10 material. > + bs->supported_write_flags = BDRV_REQ_FUA & > + bs->file->bs->supported_write_flags & > + s->test_file->bs->supported_write_flags; > + > + bs->supported_zero_flags = > + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & > + bs->file->bs->supported_zero_flags & > + s->test_file->bs->supported_zero_flags; > + > ret = 0; > fail: > qemu_opts_del(opts); >
On 08/04/2017 06:23 PM, Eric Blake wrote: > On 08/01/2017 09:18 AM, Anton Nefedov wrote: >> Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> >> --- >> block/blkverify.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) > > Basically, blkverify supports a flag if BOTH of its underlying files > also support the flag; if either side can't handle the flag, then we > fall back to emulation for both sides. > > With more overhead, we COULD state that we support both bits if at least > one of the two underlying BDS supports the bit, and then emulate support > for the bit on the second BDS where it was lacking, so that at least the > first BDS doesn't suffer from the penalties of the fallbacks. But that > means duplicating the block layer fallback code in blkverify, which is > already something that we don't necessarily expect high performance from. > > For FUA, failure to implement the bit merely means that we have more > device-wide flush calls (instead of per-transaction mini-flushes), but > the end data should be the same. But for MAY_UNMAP, I'm worried that we > may have situations where a plain BDS will create holes, while running > the same device paired through blkverify will fall back to slower > explicit zeroes. I'm wondering whether this will bite us, if we have > scenarios where the mere fact of trying to verify block device behavior > changes what behavior we are even verifying. > > Thus, while I think the code change _looks_ okay, I'm not sure if it is > correct design-wise, nor whether it is 2.10 material. > But this change doesn't make things worse, does it? Blkverify will support at least the matching bytes, while now it drops them all. Will the 'reference' driver (I guess it's raw/file in most of the cases) usually support a superset of 'tested' driver supported bits? Maybe we should report an error otherwise? >> + bs->supported_write_flags = BDRV_REQ_FUA & >> + bs->file->bs->supported_write_flags & >> + s->test_file->bs->supported_write_flags; >> + >> + bs->supported_zero_flags = >> + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & >> + bs->file->bs->supported_zero_flags & >> + s->test_file->bs->supported_zero_flags; >> + >> ret = 0; >> fail: >> qemu_opts_del(opts); >> > /Anton
diff --git a/block/blkverify.c b/block/blkverify.c index 06369f9..9ba65d0 100644 --- a/block/blkverify.c +++ b/block/blkverify.c @@ -140,6 +140,15 @@ static int blkverify_open(BlockDriverState *bs, QDict *options, int flags, goto fail; } + bs->supported_write_flags = BDRV_REQ_FUA & + bs->file->bs->supported_write_flags & + s->test_file->bs->supported_write_flags; + + bs->supported_zero_flags = + (BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP) & + bs->file->bs->supported_zero_flags & + s->test_file->bs->supported_zero_flags; + ret = 0; fail: qemu_opts_del(opts);
Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> --- block/blkverify.c | 9 +++++++++ 1 file changed, 9 insertions(+)