| Message ID | 20170524202842.26724-4-eblake@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 05/24 15:28, Eric Blake wrote: > Not all callers care about which BDS owns the mapping for a given > range of the file. This patch merely simplifies the callers by > consolidating the logic in the common call point, while guaranteeing > a non-NULL file to all the driver callbacks, for no semantic change. > > However, this will also set the stage for a future cleanup: when a > caller does not care about which BDS owns an offset, it would be > nice to allow the driver to optimize things to not have to return > BDRV_BLOCK_OFFSET_VALID in the first place. In the case of fragmented > allocation (for example, it's fairly easy to create a qcow2 image > where consecutive guest addresses are not at consecutive host > addresses), the current contract requires bdrv_get_block_status() > to clamp *pnum to the limit where host addresses are no longer > consecutive, but allowing a NULL file means that *pnum could be > set to the full length of known-allocated data. > > Signed-off-by: Eric Blake <eblake@redhat.com> > > --- > v2: new patch Yes. any particular reason why this patch is useful, besides simplifying callers? > --- > block/io.c | 15 +++++++++------ > block/mirror.c | 3 +-- > block/qcow2.c | 4 +--- > qemu-img.c | 10 ++++------ > 4 files changed, 15 insertions(+), 17 deletions(-) > > diff --git a/block/io.c b/block/io.c > index 8e6c3fe..eea74cb 100644 > --- a/block/io.c > +++ b/block/io.c > @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) > { > int64_t target_sectors, ret, nb_sectors, sector_num = 0; > BlockDriverState *bs = child->bs; > - BlockDriverState *file; > int n; > > target_sectors = bdrv_nb_sectors(bs); > @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) > if (nb_sectors <= 0) { > return 0; > } > - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file); > + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL); > if (ret < 0) { > error_report("error getting block status at sector %" PRId64 ": %s", > sector_num, strerror(-ret)); > @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData { > * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes > * beyond the end of the disk image it will be clamped. > * > - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file' > - * points to the BDS which the sector range is allocated in. > + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and > + * 'file' is non-NULL, then '*file' points to the BDS which the sector range > + * is allocated in. Sounds good. > */ > static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > int64_t sector_num, > @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, > int64_t total_sectors; > int64_t n; > int64_t ret, ret2; > + BlockDriverState *tmpfile; > > + if (!file) { > + file = &tmpfile; > + } I don't like this hunk. Instead, how about replacing all "*file = ..." with "tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning? > *file = NULL; > total_sectors = bdrv_nb_sectors(bs); > if (total_sectors < 0) { > @@ -1916,9 +1920,8 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, > int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, > int nb_sectors, int *pnum) > { > - BlockDriverState *file; > int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, > - &file); > + NULL); > if (ret < 0) { > return ret; > } > diff --git a/block/mirror.c b/block/mirror.c > index e86f8f8..4563ba7 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -392,7 +392,6 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) > while (nb_chunks > 0 && sector_num < end) { > int64_t ret; > int io_sectors, io_sectors_acct; > - BlockDriverState *file; > enum MirrorMethod { > MIRROR_METHOD_COPY, > MIRROR_METHOD_ZERO, > @@ -402,7 +401,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) > assert(!(sector_num % sectors_per_chunk)); > ret = bdrv_get_block_status_above(source, NULL, sector_num, > nb_chunks * sectors_per_chunk, > - &io_sectors, &file); > + &io_sectors, NULL); > if (ret < 0) { > io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors); > } else if (ret & BDRV_BLOCK_DATA) { > diff --git a/block/qcow2.c b/block/qcow2.c > index b3ba5da..8e9e29b 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2448,7 +2448,6 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, > uint32_t count) > { > int nr; > - BlockDriverState *file; > int64_t res; > > if (start + count > bs->total_sectors) { > @@ -2458,8 +2457,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, > if (!count) { > return true; > } > - res = bdrv_get_block_status_above(bs, NULL, start, count, > - &nr, &file); > + res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL); > return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; > } > > diff --git a/qemu-img.c b/qemu-img.c > index 5aef8ef..cb78696 100644 > --- a/qemu-img.c > +++ b/qemu-img.c > @@ -1398,7 +1398,6 @@ static int img_compare(int argc, char **argv) > > for (;;) { > int64_t status1, status2; > - BlockDriverState *file; > > nb_sectors = sectors_to_process(total_sectors, sector_num); > if (nb_sectors <= 0) { > @@ -1406,7 +1405,7 @@ static int img_compare(int argc, char **argv) > } > status1 = bdrv_get_block_status_above(bs1, NULL, sector_num, > total_sectors1 - sector_num, > - &pnum1, &file); > + &pnum1, NULL); > if (status1 < 0) { > ret = 3; > error_report("Sector allocation test failed for %s", filename1); > @@ -1416,7 +1415,7 @@ static int img_compare(int argc, char **argv) > > status2 = bdrv_get_block_status_above(bs2, NULL, sector_num, > total_sectors2 - sector_num, > - &pnum2, &file); > + &pnum2, NULL); > if (status2 < 0) { > ret = 3; > error_report("Sector allocation test failed for %s", filename2); > @@ -1615,15 +1614,14 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) > n = MIN(s->total_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS); > > if (s->sector_next_status <= sector_num) { > - BlockDriverState *file; > if (s->target_has_backing) { > ret = bdrv_get_block_status(blk_bs(s->src[src_cur]), > sector_num - src_cur_offset, > - n, &n, &file); > + n, &n, NULL); > } else { > ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL, > sector_num - src_cur_offset, > - n, &n, &file); > + n, &n, NULL); > } > if (ret < 0) { > return ret; > -- > 2.9.4 > Otherwise looks good. Fam
On 05/25/2017 01:34 AM, Fam Zheng wrote: > On Wed, 05/24 15:28, Eric Blake wrote: >> Not all callers care about which BDS owns the mapping for a given >> range of the file. This patch merely simplifies the callers by >> consolidating the logic in the common call point, while guaranteeing >> a non-NULL file to all the driver callbacks, for no semantic change. >> >> However, this will also set the stage for a future cleanup: when a >> caller does not care about which BDS owns an offset, it would be >> nice to allow the driver to optimize things to not have to return >> BDRV_BLOCK_OFFSET_VALID in the first place. In the case of fragmented >> allocation (for example, it's fairly easy to create a qcow2 image >> where consecutive guest addresses are not at consecutive host >> addresses), the current contract requires bdrv_get_block_status() >> to clamp *pnum to the limit where host addresses are no longer >> consecutive, but allowing a NULL file means that *pnum could be >> set to the full length of known-allocated data. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v2: new patch > > Yes. any particular reason why this patch is useful, besides simplifying > callers? I guess it is not directly useful to this series. I can delay it to later, and include it as part of my v2 of changing bdrv_get_block_status() to be byte-based. As mentioned in the commit message, it opens the door for a potential optimization for callers that don't care about offsets, but only allocation status, so that they can traverse the entire device address space with fewer queries. I'm fine if we want to just go with 1,2,4,5 for now. >> @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, >> int64_t total_sectors; >> int64_t n; >> int64_t ret, ret2; >> + BlockDriverState *tmpfile; >> >> + if (!file) { >> + file = &tmpfile; >> + } > > I don't like this hunk. Instead, how about replacing all "*file = ..." with > "tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning? Can do, particularly if I delay this patch to a later series, and we go with the rest now.
On 2017-05-25 08:34, Fam Zheng wrote: > On Wed, 05/24 15:28, Eric Blake wrote: >> Not all callers care about which BDS owns the mapping for a given >> range of the file. This patch merely simplifies the callers by >> consolidating the logic in the common call point, while guaranteeing >> a non-NULL file to all the driver callbacks, for no semantic change. >> >> However, this will also set the stage for a future cleanup: when a >> caller does not care about which BDS owns an offset, it would be >> nice to allow the driver to optimize things to not have to return >> BDRV_BLOCK_OFFSET_VALID in the first place. In the case of fragmented >> allocation (for example, it's fairly easy to create a qcow2 image >> where consecutive guest addresses are not at consecutive host >> addresses), the current contract requires bdrv_get_block_status() >> to clamp *pnum to the limit where host addresses are no longer >> consecutive, but allowing a NULL file means that *pnum could be >> set to the full length of known-allocated data. >> >> Signed-off-by: Eric Blake <eblake@redhat.com> >> >> --- >> v2: new patch > > Yes. any particular reason why this patch is useful, besides simplifying > callers? > > >> --- >> block/io.c | 15 +++++++++------ >> block/mirror.c | 3 +-- >> block/qcow2.c | 4 +--- >> qemu-img.c | 10 ++++------ >> 4 files changed, 15 insertions(+), 17 deletions(-) >> >> diff --git a/block/io.c b/block/io.c >> index 8e6c3fe..eea74cb 100644 >> --- a/block/io.c >> +++ b/block/io.c >> @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) >> { >> int64_t target_sectors, ret, nb_sectors, sector_num = 0; >> BlockDriverState *bs = child->bs; >> - BlockDriverState *file; >> int n; >> >> target_sectors = bdrv_nb_sectors(bs); >> @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) >> if (nb_sectors <= 0) { >> return 0; >> } >> - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file); >> + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL); >> if (ret < 0) { >> error_report("error getting block status at sector %" PRId64 ": %s", >> sector_num, strerror(-ret)); >> @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData { >> * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes >> * beyond the end of the disk image it will be clamped. >> * >> - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file' >> - * points to the BDS which the sector range is allocated in. >> + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and >> + * 'file' is non-NULL, then '*file' points to the BDS which the sector range >> + * is allocated in. > > Sounds good. > >> */ >> static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, >> int64_t sector_num, >> @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, >> int64_t total_sectors; >> int64_t n; >> int64_t ret, ret2; >> + BlockDriverState *tmpfile; >> >> + if (!file) { >> + file = &tmpfile; >> + } > > I don't like this hunk. Instead, how about replacing all "*file = ..." with > "tmpfile = ..." and add "if (file) { *file = tmpfile; }" before returning? Sounds fine to me, but in that case I'd like to request a different variable name. Maybe do what we have with errp/local_error and make it local_file or something? Max
diff --git a/block/io.c b/block/io.c index 8e6c3fe..eea74cb 100644 --- a/block/io.c +++ b/block/io.c @@ -706,7 +706,6 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) { int64_t target_sectors, ret, nb_sectors, sector_num = 0; BlockDriverState *bs = child->bs; - BlockDriverState *file; int n; target_sectors = bdrv_nb_sectors(bs); @@ -719,7 +718,7 @@ int bdrv_make_zero(BdrvChild *child, BdrvRequestFlags flags) if (nb_sectors <= 0) { return 0; } - ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, &file); + ret = bdrv_get_block_status(bs, sector_num, nb_sectors, &n, NULL); if (ret < 0) { error_report("error getting block status at sector %" PRId64 ": %s", sector_num, strerror(-ret)); @@ -1737,8 +1736,9 @@ typedef struct BdrvCoGetBlockStatusData { * 'nb_sectors' is the max value 'pnum' should be set to. If nb_sectors goes * beyond the end of the disk image it will be clamped. * - * If returned value is positive and BDRV_BLOCK_OFFSET_VALID bit is set, 'file' - * points to the BDS which the sector range is allocated in. + * If returned value is positive, BDRV_BLOCK_OFFSET_VALID bit is set, and + * 'file' is non-NULL, then '*file' points to the BDS which the sector range + * is allocated in. */ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t sector_num, @@ -1748,7 +1748,11 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs, int64_t total_sectors; int64_t n; int64_t ret, ret2; + BlockDriverState *tmpfile; + if (!file) { + file = &tmpfile; + } *file = NULL; total_sectors = bdrv_nb_sectors(bs); if (total_sectors < 0) { @@ -1916,9 +1920,8 @@ int64_t bdrv_get_block_status(BlockDriverState *bs, int coroutine_fn bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors, int *pnum) { - BlockDriverState *file; int64_t ret = bdrv_get_block_status(bs, sector_num, nb_sectors, pnum, - &file); + NULL); if (ret < 0) { return ret; } diff --git a/block/mirror.c b/block/mirror.c index e86f8f8..4563ba7 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -392,7 +392,6 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) while (nb_chunks > 0 && sector_num < end) { int64_t ret; int io_sectors, io_sectors_acct; - BlockDriverState *file; enum MirrorMethod { MIRROR_METHOD_COPY, MIRROR_METHOD_ZERO, @@ -402,7 +401,7 @@ static uint64_t coroutine_fn mirror_iteration(MirrorBlockJob *s) assert(!(sector_num % sectors_per_chunk)); ret = bdrv_get_block_status_above(source, NULL, sector_num, nb_chunks * sectors_per_chunk, - &io_sectors, &file); + &io_sectors, NULL); if (ret < 0) { io_sectors = MIN(nb_chunks * sectors_per_chunk, max_io_sectors); } else if (ret & BDRV_BLOCK_DATA) { diff --git a/block/qcow2.c b/block/qcow2.c index b3ba5da..8e9e29b 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2448,7 +2448,6 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, uint32_t count) { int nr; - BlockDriverState *file; int64_t res; if (start + count > bs->total_sectors) { @@ -2458,8 +2457,7 @@ static bool is_zero_sectors(BlockDriverState *bs, int64_t start, if (!count) { return true; } - res = bdrv_get_block_status_above(bs, NULL, start, count, - &nr, &file); + res = bdrv_get_block_status_above(bs, NULL, start, count, &nr, NULL); return res >= 0 && (res & BDRV_BLOCK_ZERO) && nr == count; } diff --git a/qemu-img.c b/qemu-img.c index 5aef8ef..cb78696 100644 --- a/qemu-img.c +++ b/qemu-img.c @@ -1398,7 +1398,6 @@ static int img_compare(int argc, char **argv) for (;;) { int64_t status1, status2; - BlockDriverState *file; nb_sectors = sectors_to_process(total_sectors, sector_num); if (nb_sectors <= 0) { @@ -1406,7 +1405,7 @@ static int img_compare(int argc, char **argv) } status1 = bdrv_get_block_status_above(bs1, NULL, sector_num, total_sectors1 - sector_num, - &pnum1, &file); + &pnum1, NULL); if (status1 < 0) { ret = 3; error_report("Sector allocation test failed for %s", filename1); @@ -1416,7 +1415,7 @@ static int img_compare(int argc, char **argv) status2 = bdrv_get_block_status_above(bs2, NULL, sector_num, total_sectors2 - sector_num, - &pnum2, &file); + &pnum2, NULL); if (status2 < 0) { ret = 3; error_report("Sector allocation test failed for %s", filename2); @@ -1615,15 +1614,14 @@ static int convert_iteration_sectors(ImgConvertState *s, int64_t sector_num) n = MIN(s->total_sectors - sector_num, BDRV_REQUEST_MAX_SECTORS); if (s->sector_next_status <= sector_num) { - BlockDriverState *file; if (s->target_has_backing) { ret = bdrv_get_block_status(blk_bs(s->src[src_cur]), sector_num - src_cur_offset, - n, &n, &file); + n, &n, NULL); } else { ret = bdrv_get_block_status_above(blk_bs(s->src[src_cur]), NULL, sector_num - src_cur_offset, - n, &n, &file); + n, &n, NULL); } if (ret < 0) { return ret;
Not all callers care about which BDS owns the mapping for a given range of the file. This patch merely simplifies the callers by consolidating the logic in the common call point, while guaranteeing a non-NULL file to all the driver callbacks, for no semantic change. However, this will also set the stage for a future cleanup: when a caller does not care about which BDS owns an offset, it would be nice to allow the driver to optimize things to not have to return BDRV_BLOCK_OFFSET_VALID in the first place. In the case of fragmented allocation (for example, it's fairly easy to create a qcow2 image where consecutive guest addresses are not at consecutive host addresses), the current contract requires bdrv_get_block_status() to clamp *pnum to the limit where host addresses are no longer consecutive, but allowing a NULL file means that *pnum could be set to the full length of known-allocated data. Signed-off-by: Eric Blake <eblake@redhat.com> --- v2: new patch --- block/io.c | 15 +++++++++------ block/mirror.c | 3 +-- block/qcow2.c | 4 +--- qemu-img.c | 10 ++++------ 4 files changed, 15 insertions(+), 17 deletions(-)