[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 20/21] block: Minimize raw use of bds->total_
From: |
Kevin Wolf |
Subject: |
Re: [Qemu-devel] [PATCH v4 20/21] block: Minimize raw use of bds->total_sectors |
Date: |
Thu, 6 Jul 2017 18:48:20 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
Am 05.07.2017 um 23:08 hat Eric Blake geschrieben:
> bdrv_is_allocated_above() was relying on intermediate->total_sectors,
> which is a field that can have stale contents depending on the value
> of intermediate->has_variable_length. An audit shows that we are safe
> (we were first calling through bdrv_co_get_block_status() which in
> turn calls bdrv_nb_sectors() and therefore just refreshed the current
> length), but it's nicer to favor our accessor functions to avoid having
> to repeat such an audit, even if it means refresh_total_sectors() is
> called more frequently.
>
> Suggested-by: John Snow <address@hidden>
> Signed-off-by: Eric Blake <address@hidden>
> Reviewed-by: Manos Pitsidianakis <address@hidden>
> Reviewed-by: Jeff Cody <address@hidden>
>
> ---
> v3-v4: no change
> v2: new patch
> ---
> block/io.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/block/io.c b/block/io.c
> index cb40069..fb8d1c7 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1952,6 +1952,7 @@ int bdrv_is_allocated_above(BlockDriverState *top,
> intermediate = top;
> while (intermediate && intermediate != base) {
> int64_t pnum_inter;
> + int64_t size_inter;
> int psectors_inter;
>
> ret = bdrv_is_allocated(intermediate, sector_num * BDRV_SECTOR_SIZE,
> @@ -1969,13 +1970,14 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>
> /*
> * [sector_num, nb_sectors] is unallocated on top but intermediate
> - * might have
> - *
> - * [sector_num+x, nr_sectors] allocated.
> + * might have [sector_num+x, nb_sectors-x] allocated.
> */
I tried to figure out what this comment wants to tell us, and neither
the original version nor your changed one seemed to make a lot of sense
to me. The only case that I can see that actually needs the following
block is a case like this:
(. = unallocated, # = allocated)
top ....####
intermediate ........
Our initial request was for 8 sectors, but when going to the
intermediate node, we need to reduce this to 4 sectors, otherwise we
would return unallocated for sectors 5 to 8 even though they are
allocated in top.
That's kind of the opposite of what the comment says, though...
> + size_inter = bdrv_nb_sectors(intermediate);
> + if (size_inter < 0) {
> + return size_inter;
> + }
> if (n > psectors_inter &&
> - (intermediate == top ||
> - sector_num + psectors_inter < intermediate->total_sectors)) {
> + (intermediate == top || sector_num + psectors_inter <
> size_inter)) {
> n = psectors_inter;
> }
The actual code change looks good.
Kevin
- [Qemu-devel] [PATCH v4 15/21] backup: Switch BackupBlockJob to byte-based, (continued)
- [Qemu-devel] [PATCH v4 15/21] backup: Switch BackupBlockJob to byte-based, Eric Blake, 2017/07/05
- [Qemu-devel] [PATCH v4 16/21] backup: Switch block_backup.h to byte-based, Eric Blake, 2017/07/05
- [Qemu-devel] [PATCH v4 17/21] backup: Switch backup_do_cow() to byte-based, Eric Blake, 2017/07/05
- [Qemu-devel] [PATCH v4 18/21] backup: Switch backup_run() to byte-based, Eric Blake, 2017/07/05
- [Qemu-devel] [PATCH v4 20/21] block: Minimize raw use of bds->total_sectors, Eric Blake, 2017/07/05
- [Qemu-devel] [PATCH v4 19/21] block: Make bdrv_is_allocated() byte-based, Eric Blake, 2017/07/05
[Qemu-devel] [PATCH v4 21/21] block: Make bdrv_is_allocated_above() byte-based, Eric Blake, 2017/07/05