| Message ID | 20170403160936.28293-10-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Max, On 04/03/2017 01:09 PM, Max Reitz wrote: > This patch adds two new parameters to the preallocate() function so we > will be able to use it not just for preallocating a new image but also > for preallocated image growth. > > The offset parameter allows the caller to specify a virtual offset from > which to start preallocating. For newly created images this is always 0, > but for preallocating growth this will be the old image length. > > The new_length parameter specifies the supposed new length of the image > (basically the "end offset" for preallocation). During image truncation, > bdrv_getlength() will return the old image length so we cannot rely on > its return value then. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index 9fd220ec34..163d51ec65 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2034,17 +2034,23 @@ static int qcow2_change_backing_file(BlockDriverState *bs, > return qcow2_update_header(bs); > } > > -static int preallocate(BlockDriverState *bs) > +/** > + * Preallocates metadata structures for data clusters between @offset (in the > + * guest disk) and @new_length (which is thus generally the new guest disk > + * size). > + * > + * Returns: 0 on success, -errno on failure. > + */ > +static int preallocate(BlockDriverState *bs, int64_t offset, int64_t new_length) > { > uint64_t bytes; > - uint64_t offset; > uint64_t host_offset = 0; > unsigned int cur_bytes; > int ret; > QCowL2Meta *meta; > > - bytes = bdrv_getlength(bs); > - offset = 0; why use `int64_t`? is it ok to have a negative offset? preallocate a negative length sound weird... even `bytes` is unsigned. > + assert(offset <= new_length); > + bytes = new_length - offset; > > while (bytes) { > cur_bytes = MIN(bytes, INT_MAX); > @@ -2314,7 +2320,7 @@ static int qcow2_create2(const char *filename, int64_t total_size, > if (prealloc != PREALLOC_MODE_OFF) { > BDRVQcow2State *s = blk_bs(blk)->opaque; > qemu_co_mutex_lock(&s->lock); > - ret = preallocate(blk_bs(blk)); > + ret = preallocate(blk_bs(blk), 0, total_size); > qemu_co_mutex_unlock(&s->lock); > if (ret < 0) { > error_setg_errno(errp, -ret, "Could not preallocate metadata"); >
On 03.04.2017 21:19, Philippe Mathieu-Daudé wrote: > Hi Max, > > On 04/03/2017 01:09 PM, Max Reitz wrote: >> This patch adds two new parameters to the preallocate() function so we >> will be able to use it not just for preallocating a new image but also >> for preallocated image growth. >> >> The offset parameter allows the caller to specify a virtual offset from >> which to start preallocating. For newly created images this is always 0, >> but for preallocating growth this will be the old image length. >> >> The new_length parameter specifies the supposed new length of the image >> (basically the "end offset" for preallocation). During image truncation, >> bdrv_getlength() will return the old image length so we cannot rely on >> its return value then. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/qcow2.c | 16 +++++++++++----- >> 1 file changed, 11 insertions(+), 5 deletions(-) >> >> diff --git a/block/qcow2.c b/block/qcow2.c >> index 9fd220ec34..163d51ec65 100644 >> --- a/block/qcow2.c >> +++ b/block/qcow2.c >> @@ -2034,17 +2034,23 @@ static int >> qcow2_change_backing_file(BlockDriverState *bs, >> return qcow2_update_header(bs); >> } >> >> -static int preallocate(BlockDriverState *bs) >> +/** >> + * Preallocates metadata structures for data clusters between @offset >> (in the >> + * guest disk) and @new_length (which is thus generally the new guest >> disk >> + * size). >> + * >> + * Returns: 0 on success, -errno on failure. >> + */ >> +static int preallocate(BlockDriverState *bs, int64_t offset, int64_t >> new_length) >> { >> uint64_t bytes; >> - uint64_t offset; >> uint64_t host_offset = 0; >> unsigned int cur_bytes; >> int ret; >> QCowL2Meta *meta; >> >> - bytes = bdrv_getlength(bs); >> - offset = 0; > > why use `int64_t`? is it ok to have a negative offset? > preallocate a negative length sound weird... even `bytes` is unsigned. Right. That's actually a very good question. I can't remember why I chose them to be signed. Generally, it won't be an issue because the QEMU block layer will generally not permit files to be longer than 2^63 (e.g. bdrv_co_pwritev() and bdrv_co_preadv() take int64_t offsets). But you're right, there is no reason for those parameters to be signed here. I'll make them unsigned, thanks! Max >> + assert(offset <= new_length); >> + bytes = new_length - offset; >> >> while (bytes) { >> cur_bytes = MIN(bytes, INT_MAX); >> @@ -2314,7 +2320,7 @@ static int qcow2_create2(const char *filename, >> int64_t total_size, >> if (prealloc != PREALLOC_MODE_OFF) { >> BDRVQcow2State *s = blk_bs(blk)->opaque; >> qemu_co_mutex_lock(&s->lock); >> - ret = preallocate(blk_bs(blk)); >> + ret = preallocate(blk_bs(blk), 0, total_size); >> qemu_co_mutex_unlock(&s->lock); >> if (ret < 0) { >> error_setg_errno(errp, -ret, "Could not preallocate >> metadata"); >>
On Mon, Apr 03, 2017 at 06:09:29PM +0200, Max Reitz wrote: > This patch adds two new parameters to the preallocate() function so we > will be able to use it not just for preallocating a new image but also > for preallocated image growth. > > The offset parameter allows the caller to specify a virtual offset from > which to start preallocating. For newly created images this is always 0, > but for preallocating growth this will be the old image length. > > The new_length parameter specifies the supposed new length of the image > (basically the "end offset" for preallocation). During image truncation, > bdrv_getlength() will return the old image length so we cannot rely on > its return value then. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.c | 16 +++++++++++----- > 1 file changed, 11 insertions(+), 5 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
diff --git a/block/qcow2.c b/block/qcow2.c index 9fd220ec34..163d51ec65 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2034,17 +2034,23 @@ static int qcow2_change_backing_file(BlockDriverState *bs, return qcow2_update_header(bs); } -static int preallocate(BlockDriverState *bs) +/** + * Preallocates metadata structures for data clusters between @offset (in the + * guest disk) and @new_length (which is thus generally the new guest disk + * size). + * + * Returns: 0 on success, -errno on failure. + */ +static int preallocate(BlockDriverState *bs, int64_t offset, int64_t new_length) { uint64_t bytes; - uint64_t offset; uint64_t host_offset = 0; unsigned int cur_bytes; int ret; QCowL2Meta *meta; - bytes = bdrv_getlength(bs); - offset = 0; + assert(offset <= new_length); + bytes = new_length - offset; while (bytes) { cur_bytes = MIN(bytes, INT_MAX); @@ -2314,7 +2320,7 @@ static int qcow2_create2(const char *filename, int64_t total_size, if (prealloc != PREALLOC_MODE_OFF) { BDRVQcow2State *s = blk_bs(blk)->opaque; qemu_co_mutex_lock(&s->lock); - ret = preallocate(blk_bs(blk)); + ret = preallocate(blk_bs(blk), 0, total_size); qemu_co_mutex_unlock(&s->lock); if (ret < 0) { error_setg_errno(errp, -ret, "Could not preallocate metadata");
This patch adds two new parameters to the preallocate() function so we will be able to use it not just for preallocating a new image but also for preallocated image growth. The offset parameter allows the caller to specify a virtual offset from which to start preallocating. For newly created images this is always 0, but for preallocating growth this will be the old image length. The new_length parameter specifies the supposed new length of the image (basically the "end offset" for preallocation). During image truncation, bdrv_getlength() will return the old image length so we cannot rely on its return value then. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2.c | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-)