| Message ID | 20171009215533.12530-3-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 2017-10-09 23:55, Max Reitz wrote: > Some qcow2 functions (at least perform_cow()) expect s->lock to be > taken. Therefore, if we want to make use of them, we should execute > preallocate() (as "preallocate_co") in a coroutine so that we can use > the qemu_co_mutex_* functions. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 34 insertions(+), 7 deletions(-) Cc: qemu-stable@nongnu.org
On 10/09/2017 04:55 PM, Max Reitz wrote: > Some qcow2 functions (at least perform_cow()) expect s->lock to be > taken. Therefore, if we want to make use of them, we should execute > preallocate() (as "preallocate_co") in a coroutine so that we can use > the qemu_co_mutex_* functions. > > Signed-off-by: Max Reitz <mreitz@redhat.com> > --- > block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 34 insertions(+), 7 deletions(-) > > +++ b/block/qcow2.c > @@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, > } > > > +typedef struct PreallocCo { > + BlockDriverState *bs; > + uint64_t offset; > + uint64_t new_length; > + > + int ret; > +} PreallocCo; Because you create a typedef here... > +static void coroutine_fn preallocate_co(void *opaque) > { > + struct PreallocCo *params = opaque; you could drop 'struct' here, the way you already dropped it... > +static int preallocate(BlockDriverState *bs, > + uint64_t offset, uint64_t new_length) > +{ > + PreallocCo params = { > + .bs = bs, > + .offset = offset, > + .new_length = new_length, > + .ret = -EINPROGRESS, > + }; ...here. But that doesn't change semantics, so either way, Reviewed-by: Eric Blake <eblake@redhat.com>
On Mon, Oct 09, 2017 at 11:55:32PM +0200, Max Reitz wrote: > Some qcow2 functions (at least perform_cow()) expect s->lock to be > taken. Therefore, if we want to make use of them, we should execute > preallocate() (as "preallocate_co") in a coroutine so that we can use > the qemu_co_mutex_* functions. > > Signed-off-by: Max Reitz <mreitz@redhat.com> Reviewed-by: Jeff Cody <jcody@redhat.com> > --- > block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++------- > 1 file changed, 34 insertions(+), 7 deletions(-) > > diff --git a/block/qcow2.c b/block/qcow2.c > index c3b312cdef..cc75a5167f 100644 > --- a/block/qcow2.c > +++ b/block/qcow2.c > @@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, > } > > > +typedef struct PreallocCo { > + BlockDriverState *bs; > + uint64_t offset; > + uint64_t new_length; > + > + int ret; > +} PreallocCo; > + > /** > * Preallocates metadata structures for data clusters between @offset (in the > * guest disk) and @new_length (which is thus generally the new guest disk > @@ -2467,9 +2475,12 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, > * > * Returns: 0 on success, -errno on failure. > */ > -static int preallocate(BlockDriverState *bs, > - uint64_t offset, uint64_t new_length) > +static void coroutine_fn preallocate_co(void *opaque) > { > + struct PreallocCo *params = opaque; > + BlockDriverState *bs = params->bs; > + uint64_t offset = params->offset; > + uint64_t new_length = params->new_length; > BDRVQcow2State *s = bs->opaque; > uint64_t bytes; > uint64_t host_offset = 0; > @@ -2477,9 +2488,7 @@ static int preallocate(BlockDriverState *bs, > int ret; > QCowL2Meta *meta; > > - if (qemu_in_coroutine()) { > - qemu_co_mutex_lock(&s->lock); > - } > + qemu_co_mutex_lock(&s->lock); > > assert(offset <= new_length); > bytes = new_length - offset; > @@ -2533,10 +2542,28 @@ static int preallocate(BlockDriverState *bs, > ret = 0; > > done: > + qemu_co_mutex_unlock(&s->lock); > + params->ret = ret; > +} > + > +static int preallocate(BlockDriverState *bs, > + uint64_t offset, uint64_t new_length) > +{ > + PreallocCo params = { > + .bs = bs, > + .offset = offset, > + .new_length = new_length, > + .ret = -EINPROGRESS, > + }; > + > if (qemu_in_coroutine()) { > - qemu_co_mutex_unlock(&s->lock); > + preallocate_co(¶ms); > + } else { > + Coroutine *co = qemu_coroutine_create(preallocate_co, ¶ms); > + bdrv_coroutine_enter(bs, co); > + BDRV_POLL_WHILE(bs, params.ret == -EINPROGRESS); > } > - return ret; > + return params.ret; > } > > /* qcow2_refcount_metadata_size: > -- > 2.13.6 > >
On 2017-10-10 00:28, Eric Blake wrote: > On 10/09/2017 04:55 PM, Max Reitz wrote: >> Some qcow2 functions (at least perform_cow()) expect s->lock to be >> taken. Therefore, if we want to make use of them, we should execute >> preallocate() (as "preallocate_co") in a coroutine so that we can use >> the qemu_co_mutex_* functions. >> >> Signed-off-by: Max Reitz <mreitz@redhat.com> >> --- >> block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++------- >> 1 file changed, 34 insertions(+), 7 deletions(-) >> > >> +++ b/block/qcow2.c >> @@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, >> } >> >> >> +typedef struct PreallocCo { >> + BlockDriverState *bs; >> + uint64_t offset; >> + uint64_t new_length; >> + >> + int ret; >> +} PreallocCo; > > Because you create a typedef here... > > >> +static void coroutine_fn preallocate_co(void *opaque) >> { >> + struct PreallocCo *params = opaque; > > you could drop 'struct' here, the way you already dropped it... Ah, yes, that was from before I added the typedef... Will fix. Max > >> +static int preallocate(BlockDriverState *bs, >> + uint64_t offset, uint64_t new_length) >> +{ >> + PreallocCo params = { >> + .bs = bs, >> + .offset = offset, >> + .new_length = new_length, >> + .ret = -EINPROGRESS, >> + }; > > ...here. But that doesn't change semantics, so either way, > > Reviewed-by: Eric Blake <eblake@redhat.com> >
diff --git a/block/qcow2.c b/block/qcow2.c index c3b312cdef..cc75a5167f 100644 --- a/block/qcow2.c +++ b/block/qcow2.c @@ -2460,6 +2460,14 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, } +typedef struct PreallocCo { + BlockDriverState *bs; + uint64_t offset; + uint64_t new_length; + + int ret; +} PreallocCo; + /** * Preallocates metadata structures for data clusters between @offset (in the * guest disk) and @new_length (which is thus generally the new guest disk @@ -2467,9 +2475,12 @@ static int qcow2_set_up_encryption(BlockDriverState *bs, const char *encryptfmt, * * Returns: 0 on success, -errno on failure. */ -static int preallocate(BlockDriverState *bs, - uint64_t offset, uint64_t new_length) +static void coroutine_fn preallocate_co(void *opaque) { + struct PreallocCo *params = opaque; + BlockDriverState *bs = params->bs; + uint64_t offset = params->offset; + uint64_t new_length = params->new_length; BDRVQcow2State *s = bs->opaque; uint64_t bytes; uint64_t host_offset = 0; @@ -2477,9 +2488,7 @@ static int preallocate(BlockDriverState *bs, int ret; QCowL2Meta *meta; - if (qemu_in_coroutine()) { - qemu_co_mutex_lock(&s->lock); - } + qemu_co_mutex_lock(&s->lock); assert(offset <= new_length); bytes = new_length - offset; @@ -2533,10 +2542,28 @@ static int preallocate(BlockDriverState *bs, ret = 0; done: + qemu_co_mutex_unlock(&s->lock); + params->ret = ret; +} + +static int preallocate(BlockDriverState *bs, + uint64_t offset, uint64_t new_length) +{ + PreallocCo params = { + .bs = bs, + .offset = offset, + .new_length = new_length, + .ret = -EINPROGRESS, + }; + if (qemu_in_coroutine()) { - qemu_co_mutex_unlock(&s->lock); + preallocate_co(¶ms); + } else { + Coroutine *co = qemu_coroutine_create(preallocate_co, ¶ms); + bdrv_coroutine_enter(bs, co); + BDRV_POLL_WHILE(bs, params.ret == -EINPROGRESS); } - return ret; + return params.ret; } /* qcow2_refcount_metadata_size:
Some qcow2 functions (at least perform_cow()) expect s->lock to be taken. Therefore, if we want to make use of them, we should execute preallocate() (as "preallocate_co") in a coroutine so that we can use the qemu_co_mutex_* functions. Signed-off-by: Max Reitz <mreitz@redhat.com> --- block/qcow2.c | 41 ++++++++++++++++++++++++++++++++++------- 1 file changed, 34 insertions(+), 7 deletions(-)