[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-block] [PATCH 00/17] make bdrv_is_allocated[_abov
Re: [Qemu-devel] [Qemu-block] [PATCH 00/17] make bdrv_is_allocated[_above] byte-based
Mon, 17 Apr 2017 20:04:35 -0500
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.0
On 04/17/2017 06:42 PM, John Snow wrote:
> On 04/11/2017 06:29 PM, Eric Blake wrote:
>> There are patches floating around to add NBD_CMD_BLOCK_STATUS,
>> but NBD wants to report status on byte granularity (even if the
>> reporting will probably be naturally aligned to sectors or even
>> much higher levels). I've therefore started the task of
>> converting our block status code to report at a byte granularity
>> rather than sectors.
>> This is part one of that conversion: bdrv_is_allocated().
>> Other parts (still to be written) include tracking dirty bitmaps
>> by bytes (it's still one bit per granularity, but now we won't
>> be double-scaling from bytes to sectors to granularity),
That series is not only written, but reviewed now:
>> replacing bdrv_get_block_status() with a byte based callback
>> in all the drivers.
Coming up soon.
>> Available as a tag at:
>> git fetch git://repo.or.cz/qemu/ericb.git nbd-byte-allocated-v1
>> It requires v9 or later of my prior work on blkdebug:
>> which in turn requires Max's block-next tree:
>> include/block/block.h | 6 +-
>> include/block/block_backup.h | 11 +-
>> include/qemu/ratelimit.h | 3 +-
>> block/backup.c | 126 ++++++++----------
>> block/commit.c | 54 ++++----
>> block/io.c | 59 +++++----
>> block/mirror.c | 300
>> block/replication.c | 29 +++--
>> block/stream.c | 35 +++--
>> block/vvfat.c | 34 +++--
>> migration/block.c | 9 +-
>> qemu-img.c | 15 ++-
>> qemu-io-cmds.c | 57 ++++----
>> block/trace-events | 14 +-
>> 14 files changed, 381 insertions(+), 371 deletions(-)
> Shame, you added ten lines!
Yeah, some of that back-and-forth scaling is verbose and resulted in
line breaks that used to fit in one. Of course, the second series
regained those ten lines and more, once I got to flatten some of the
interfaces away from repeated scaling:
$ git diff --stat nbd-byte-allocated-v1..nbd-byte-dirty-v1 | cat
include/block/block_int.h | 2 +-
include/block/dirty-bitmap.h | 21 ++++-------
block/backup.c | 7 ++--
block/dirty-bitmap.c | 83
block/io.c | 6 ++--
block/mirror.c | 73 +++++++++++++++++---------------------
migration/block.c | 14 ++++----
7 files changed, 74 insertions(+), 132 deletions(-)
> Patches 1-15:
> Reviewed-by: John Snow <address@hidden>
> 9: Is there a good reason for a void fn() to return its argument via a
> passed parameter? I see you're matching the other interface, but that
> strikes me as wonky.
It bothered me a bit too; beyond the copy-and-paste factor, my
justification was that the parameter is both input and output, rather
than output-only. But I don't mind respinning to add a preliminary patch
that fixes mirror_clip_sectors() to return a value, then pattern
mirror_clip_bytes to do likewise.
> 11: Looks correct to me, but this one's a bit hairier than the rest. How
> many times do we truly need to round, adjust, clip, round again, align,
> clip, round, align, ...
I don't know that there are that many rounds of clipping going on, but
there is definitely a lot of scaling, and it gets better as later
patches make even more things be byte-based.
> I'll take a peek at the last two tomorrow.
Thanks for plodding through it so far. For supposedly being no semantic
change, it is still definitely a lot to think about. But the end result
is more legible, in my opinion.
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
Description: OpenPGP digital signature
- [Qemu-devel] [PATCH 11/17] mirror: Switch mirror_iteration() to byte-based, (continued)
- [Qemu-devel] [PATCH 11/17] mirror: Switch mirror_iteration() to byte-based, Eric Blake, 2017/04/11
- [Qemu-devel] [PATCH 12/17] backup: Switch BackupBlockJob to byte-based, Eric Blake, 2017/04/11
- [Qemu-devel] [PATCH 15/17] backup: Switch backup_run() to byte-based, Eric Blake, 2017/04/11
- [Qemu-devel] [PATCH 14/17] backup: Switch backup_do_cow() to byte-based, Eric Blake, 2017/04/11
- [Qemu-devel] [PATCH 13/17] backup: Switch block_backup.h to byte-based, Eric Blake, 2017/04/11
- [Qemu-devel] [PATCH 16/17] block: Make bdrv_is_allocated() byte-based, Eric Blake, 2017/04/11
- [Qemu-devel] [PATCH 17/17] block: Make bdrv_is_allocated_above() byte-based, Eric Blake, 2017/04/11
- Re: [Qemu-devel] [Qemu-block] [PATCH 00/17] make bdrv_is_allocated[_above] byte-based, John Snow, 2017/04/17
- Re: [Qemu-devel] [Qemu-block] [PATCH 00/17] make bdrv_is_allocated[_above] byte-based,
Eric Blake <=