| Message ID | 1495186480-114192-3-git-send-email-anton.nefedov@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
On 05/19/2017 04:34 AM, Anton Nefedov wrote: > in such case, bdrv_get_block_status() shall return 0, *nr == 0 Sounds better as s/shall/will/ > > iotest 154 updated accordingly: write-zeroes tail alignment can be detected > as zeroes now, so pwrite_zeroes succeeds > > Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> > --- > block/qcow2.c | 6 ++++-- > tests/qemu-iotests/154.out | 4 ++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > You'll want to also patch test 154 proper to remove these two TODO comments: # TODO: Note that this forces an allocation, because we aren't yet able to # quickly detect that reads beyond EOF of the backing file are always zero > diff --git a/block/qcow2.c b/block/qcow2.c > index 2e6a0ec..b885dfc 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, > int64_t res; > > if (start + count > bs->total_sectors) { > - count = bs->total_sectors - start; > + count = start < bs->total_sectors ? bs->total_sectors - start : 0; > } > > if (!count) { > @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, > } > res = bdrv_get_block_status_above(bs, NULL, start, count, > &nr, &file); > - return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; > + return res >= 0 > + && (((res & BDRV_BLOCK_ZERO) && nr == count) > + || nr == 0); The logic makes sense to me, although the formatting is unusual (we tend to split && and || with the operator at the tail of the previous line, not the head of the new line). This quick check may make me revisit whether it is worth my my RFC series about adding BDRV_BLOCK_EOF for more quickly tracking reads beyond EOF (my solution in that series was able to make the same change to test 154, but by doing it at the block layer instead of the qcow2.c code). https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html
On 05/22/2017 02:12 PM, Eric Blake wrote: >> +++ b/block/qcow2.c >> @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, >> int64_t res; >> >> if (start + count > bs->total_sectors) { >> - count = bs->total_sectors - start; >> + count = start < bs->total_sectors ? bs->total_sectors - start : 0; >> } >> >> if (!count) { >> @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, >> } >> res = bdrv_get_block_status_above(bs, NULL, start, count, >> &nr, &file); >> - return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; >> + return res >= 0 >> + && (((res & BDRV_BLOCK_ZERO) && nr == count) >> + || nr == 0); > > The logic makes sense to me, although the formatting is unusual (we tend > to split && and || with the operator at the tail of the previous line, > not the head of the new line). > > This quick check may make me revisit whether it is worth my my RFC > series about adding BDRV_BLOCK_EOF for more quickly tracking reads > beyond EOF (my solution in that series was able to make the same change > to test 154, but by doing it at the block layer instead of the qcow2.c > code). https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html Actually, re-reading my RFC - I was able to change 6 instances in test 154, while your tweak only affected 2 instances (you still left four instances that were allocating). So my approach may still be more optimal in the long run.
On 05/22/2017 10:14 PM, Eric Blake wrote: > On 05/22/2017 02:12 PM, Eric Blake wrote: > >>> +++ b/block/qcow2.c >>> @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, >>> int64_t res; >>> >>> if (start + count > bs->total_sectors) { >>> - count = bs->total_sectors - start; >>> + count = start < bs->total_sectors ? bs->total_sectors - start : 0; >>> } >>> >>> if (!count) { >>> @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, >>> } >>> res = bdrv_get_block_status_above(bs, NULL, start, count, >>> &nr, &file); >>> - return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; >>> + return res >= 0 >>> + && (((res & BDRV_BLOCK_ZERO) && nr == count) >>> + || nr == 0); >> >> The logic makes sense to me, although the formatting is unusual (we tend >> to split && and || with the operator at the tail of the previous line, >> not the head of the new line). >> >> This quick check may make me revisit whether it is worth my my RFC >> series about adding BDRV_BLOCK_EOF for more quickly tracking reads >> beyond EOF (my solution in that series was able to make the same change >> to test 154, but by doing it at the block layer instead of the qcow2.c >> code). https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg01127.html > > Actually, re-reading my RFC - I was able to change 6 instances in test > 154, while your tweak only affected 2 instances (you still left four > instances that were allocating). So my approach may still be more > optimal in the long run. > Yes, looks like your approach is more general; let's drop this one then /Anton
diff --git a/block/qcow2.c b/block/qcow2.c index 2e6a0ec..b885dfc 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2482,7 +2482,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, int64_t res; if (start + count > bs->total_sectors) { - count = bs->total_sectors - start; + count = start < bs->total_sectors ? bs->total_sectors - start : 0; } if (!count) { @@ -2490,7 +2490,9 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, } res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, &file); - return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; + return res >= 0 + && (((res & BDRV_BLOCK_ZERO) && nr == count) + || nr == 0); } static coroutine_fn int qcow2_co_pwrite_zeroes(BlockDriverState *bs, diff --git a/tests/qemu-iotests/154.out b/tests/qemu-iotests/154.out index d8485ee..259340e 100644 --- a/tests/qemu-iotests/154.out +++ b/tests/qemu-iotests/154.out @@ -322,7 +322,7 @@ wrote 1024/1024 bytes at offset 134218240 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) 2048/2048 bytes allocated at offset 128 MiB [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false}, -{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}] +{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}] Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base wrote 2048/2048 bytes at offset 134217728 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) @@ -348,7 +348,7 @@ wrote 1024/1024 bytes at offset 134218240 1 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec) 2048/2048 bytes allocated at offset 128 MiB [{ "start": 0, "length": 134217728, "depth": 1, "zero": true, "data": false}, -{ "start": 134217728, "length": 2048, "depth": 0, "zero": false, "data": true, "offset": OFFSET}] +{ "start": 134217728, "length": 2048, "depth": 0, "zero": true, "data": false}] Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134219776 backing_file=TEST_DIR/t.IMGFMT.base wrote 2048/2048 bytes at offset 134217728 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
in such case, bdrv_get_block_status() shall return 0, *nr == 0 iotest 154 updated accordingly: write-zeroes tail alignment can be detected as zeroes now, so pwrite_zeroes succeeds Signed-off-by: Anton Nefedov <anton.nefedov@virtuozzo.com> --- block/qcow2.c | 6 ++++-- tests/qemu-iotests/154.out | 4 ++-- 2 files changed, 6 insertions(+), 4 deletions(-)