Re: [Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

2017-05-31 Thread Max Reitz 
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 
>>
>> ---
>> 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, , );
>> +ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , 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 = 
>> +}
> 
> 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



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

2017-05-25 Thread Eric Blake 
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 
>>
>> ---
>> 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 = 
>> +}
> 
> 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.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.   +1-919-301-3266
Virtualization:  qemu.org | libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

2017-05-25 Thread Fam Zheng 
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 
> 
> ---
> 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, , );
> +ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , 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 = 
> +}

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,
> -);
> +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,
> -  _sectors, );
> +  _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
>

[Qemu-devel] [PATCH v2 3/5] block: Allow NULL file for bdrv_get_block_status()

2017-05-24 Thread Eric Blake 
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 

---
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(-)

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, , );
+ret = bdrv_get_block_status(bs, sector_num, nb_sectors, , 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 = 
+}
 *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,
-);
+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,
-  _sectors, );
+  _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,
-