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: https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg02163.html >> then >> 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: >> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01723.html >> which in turn requires Max's block-next tree: >> https://lists.gnu.org/archive/html/qemu-devel/2017-04/msg01298.html >> >> 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 > > 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