Re: [Qemu-block] [PATCH v3 18/20] block: Make bdrv_is_allocated() byte-based
On Tue, Jun 27, 2017 at 02:24:56PM -0500, Eric Blake wrote:
> We are gradually moving away from sector-based interfaces, towards
> byte-based. In the common case, allocation is unlikely to ever use
> values that are not naturally sector-aligned, but it is possible
> that byte-based values will let us be more precise about allocation
> at the end of an unaligned file that can do byte-based access.
>
> Changing the signature of the function to use int64_t *pnum ensures
> that the compiler enforces that all callers are updated. For now,
> the io.c layer still assert()s that all callers are sector-aligned
> on input and that *pnum is sector-aligned on return to the caller,
> but that can be relaxed when a later patch implements byte-based
> block status. Therefore, this code adds usages like
> DIV_ROUND_UP(,BDRV_SECTOR_SIZE) to callers that still want aligned
> values, where the call might reasonbly give non-aligned results
> in the future; on the other hand, no rounding is needed for callers
> that should just continue to work with byte alignment.
>
> For the most part this patch is just the addition of scaling at the
> callers followed by inverse scaling at bdrv_is_allocated(). But
> some code, particularly bdrv_commit(), gets a lot simpler because it
> no longer has to mess with sectors; also, it is now possible to pass
> NULL if the caller does not care how much of the image is allocated
> beyond the initial offset.
>
> For ease of review, bdrv_is_allocated_above() will be tackled
> separately.
>
> Signed-off-by: Eric Blake
> Reviewed-by: John Snow
>
Reviewed-by: Jeff Cody
> ---
> v2: rebase to earlier changes, tweak commit message
> ---
> include/block/block.h | 4 +--
> block/backup.c| 17 -
> block/commit.c| 21 +++-
> block/io.c| 49 +---
> block/stream.c| 5 ++--
> block/vvfat.c | 34 ++---
> migration/block.c | 9 ---
> qemu-img.c| 5 +++-
> qemu-io-cmds.c| 70
> +++
> 9 files changed, 114 insertions(+), 100 deletions(-)
>
> diff --git a/include/block/block.h b/include/block/block.h
> index 5cdd690..9b9d87b 100644
> --- a/include/block/block.h
> +++ b/include/block/block.h
> @@ -425,8 +425,8 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
> int64_t sector_num,
> int nb_sectors, int *pnum,
> BlockDriverState **file);
> -int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int
> nb_sectors,
> - int *pnum);
> +int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
> + int64_t *pnum);
> int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
> int64_t sector_num, int nb_sectors, int *pnum);
>
> diff --git a/block/backup.c b/block/backup.c
> index 04def91..b2048bf 100644
> --- a/block/backup.c
> +++ b/block/backup.c
> @@ -47,12 +47,6 @@ typedef struct BackupBlockJob {
> QLIST_HEAD(, CowRequest) inflight_reqs;
> } BackupBlockJob;
>
> -/* Size of a cluster in sectors, instead of bytes. */
> -static inline int64_t cluster_size_sectors(BackupBlockJob *job)
> -{
> - return job->cluster_size / BDRV_SECTOR_SIZE;
> -}
> -
> /* See if in-flight requests overlap and wait for them to complete */
> static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
> int64_t offset,
> @@ -433,7 +427,6 @@ static void coroutine_fn backup_run(void *opaque)
> BackupCompleteData *data;
> BlockDriverState *bs = blk_bs(job->common.blk);
> int64_t offset;
> -int64_t sectors_per_cluster = cluster_size_sectors(job);
> int ret = 0;
>
> QLIST_INIT(&job->inflight_reqs);
> @@ -465,12 +458,13 @@ static void coroutine_fn backup_run(void *opaque)
> }
>
> if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
> -int i, n;
> +int i;
> +int64_t n;
>
> /* Check to see if these blocks are already in the
> * backing file. */
>
> -for (i = 0; i < sectors_per_cluster;) {
> +for (i = 0; i < job->cluster_size;) {
> /* bdrv_is_allocated() only returns true/false based
> * on the first set of sectors it comes across that
> * are are all in the same state.
> @@ -478,9 +472,8 @@ static void coroutine_fn backup_run(void *opaque)
> * backup cluster length. We end up copying more than
> * needed but at some point that is always the case. */
> alloced =
> -bdrv_is_allocated(bs,
> -
Re: [Qemu-block] [PATCH v3 18/20] block: Make bdrv_is_allocated() byte-based
Eric Blake wrote: > We are gradually moving away from sector-based interfaces, towards > byte-based. In the common case, allocation is unlikely to ever use > values that are not naturally sector-aligned, but it is possible > that byte-based values will let us be more precise about allocation > at the end of an unaligned file that can do byte-based access. > > Changing the signature of the function to use int64_t *pnum ensures > that the compiler enforces that all callers are updated. For now, > the io.c layer still assert()s that all callers are sector-aligned > on input and that *pnum is sector-aligned on return to the caller, > but that can be relaxed when a later patch implements byte-based > block status. Therefore, this code adds usages like > DIV_ROUND_UP(,BDRV_SECTOR_SIZE) to callers that still want aligned > values, where the call might reasonbly give non-aligned results > in the future; on the other hand, no rounding is needed for callers > that should just continue to work with byte alignment. > > For the most part this patch is just the addition of scaling at the > callers followed by inverse scaling at bdrv_is_allocated(). But > some code, particularly bdrv_commit(), gets a lot simpler because it > no longer has to mess with sectors; also, it is now possible to pass > NULL if the caller does not care how much of the image is allocated > beyond the initial offset. > > For ease of review, bdrv_is_allocated_above() will be tackled > separately. > > Signed-off-by: Eric Blake > Reviewed-by: John Snow Reviewed-by: Juan Quintela
[Qemu-block] [PATCH v3 18/20] block: Make bdrv_is_allocated() byte-based
We are gradually moving away from sector-based interfaces, towards
byte-based. In the common case, allocation is unlikely to ever use
values that are not naturally sector-aligned, but it is possible
that byte-based values will let us be more precise about allocation
at the end of an unaligned file that can do byte-based access.
Changing the signature of the function to use int64_t *pnum ensures
that the compiler enforces that all callers are updated. For now,
the io.c layer still assert()s that all callers are sector-aligned
on input and that *pnum is sector-aligned on return to the caller,
but that can be relaxed when a later patch implements byte-based
block status. Therefore, this code adds usages like
DIV_ROUND_UP(,BDRV_SECTOR_SIZE) to callers that still want aligned
values, where the call might reasonbly give non-aligned results
in the future; on the other hand, no rounding is needed for callers
that should just continue to work with byte alignment.
For the most part this patch is just the addition of scaling at the
callers followed by inverse scaling at bdrv_is_allocated(). But
some code, particularly bdrv_commit(), gets a lot simpler because it
no longer has to mess with sectors; also, it is now possible to pass
NULL if the caller does not care how much of the image is allocated
beyond the initial offset.
For ease of review, bdrv_is_allocated_above() will be tackled
separately.
Signed-off-by: Eric Blake
Reviewed-by: John Snow
---
v2: rebase to earlier changes, tweak commit message
---
include/block/block.h | 4 +--
block/backup.c| 17 -
block/commit.c| 21 +++-
block/io.c| 49 +---
block/stream.c| 5 ++--
block/vvfat.c | 34 ++---
migration/block.c | 9 ---
qemu-img.c| 5 +++-
qemu-io-cmds.c| 70 +++
9 files changed, 114 insertions(+), 100 deletions(-)
diff --git a/include/block/block.h b/include/block/block.h
index 5cdd690..9b9d87b 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -425,8 +425,8 @@ int64_t bdrv_get_block_status_above(BlockDriverState *bs,
int64_t sector_num,
int nb_sectors, int *pnum,
BlockDriverState **file);
-int bdrv_is_allocated(BlockDriverState *bs, int64_t sector_num, int nb_sectors,
- int *pnum);
+int bdrv_is_allocated(BlockDriverState *bs, int64_t offset, int64_t bytes,
+ int64_t *pnum);
int bdrv_is_allocated_above(BlockDriverState *top, BlockDriverState *base,
int64_t sector_num, int nb_sectors, int *pnum);
diff --git a/block/backup.c b/block/backup.c
index 04def91..b2048bf 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -47,12 +47,6 @@ typedef struct BackupBlockJob {
QLIST_HEAD(, CowRequest) inflight_reqs;
} BackupBlockJob;
-/* Size of a cluster in sectors, instead of bytes. */
-static inline int64_t cluster_size_sectors(BackupBlockJob *job)
-{
- return job->cluster_size / BDRV_SECTOR_SIZE;
-}
-
/* See if in-flight requests overlap and wait for them to complete */
static void coroutine_fn wait_for_overlapping_requests(BackupBlockJob *job,
int64_t offset,
@@ -433,7 +427,6 @@ static void coroutine_fn backup_run(void *opaque)
BackupCompleteData *data;
BlockDriverState *bs = blk_bs(job->common.blk);
int64_t offset;
-int64_t sectors_per_cluster = cluster_size_sectors(job);
int ret = 0;
QLIST_INIT(&job->inflight_reqs);
@@ -465,12 +458,13 @@ static void coroutine_fn backup_run(void *opaque)
}
if (job->sync_mode == MIRROR_SYNC_MODE_TOP) {
-int i, n;
+int i;
+int64_t n;
/* Check to see if these blocks are already in the
* backing file. */
-for (i = 0; i < sectors_per_cluster;) {
+for (i = 0; i < job->cluster_size;) {
/* bdrv_is_allocated() only returns true/false based
* on the first set of sectors it comes across that
* are are all in the same state.
@@ -478,9 +472,8 @@ static void coroutine_fn backup_run(void *opaque)
* backup cluster length. We end up copying more than
* needed but at some point that is always the case. */
alloced =
-bdrv_is_allocated(bs,
- (offset >> BDRV_SECTOR_BITS) + i,
- sectors_per_cluster - i, &n);
+bdrv_is_allocated(bs, offset + i,
+ job->cluster_size - i, &n);
i += n;
if
