Message ID | 20200601181118.579-5-vsementsov@virtuozzo.com |
---|---|
State | New |
Headers | show |
Series | backup performance: block_status + async | expand |
On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote: > Refactor common path to use BlockCopyCallState pointer as parameter, to > prepare it for use in asynchronous block-copy (at least, we'll need to > run block-copy in a coroutine, passing the whole parameters as one > pointer). > > Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> > --- > block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------ > 1 file changed, 38 insertions(+), 13 deletions(-) > > diff --git a/block/block-copy.c b/block/block-copy.c > index 43a018d190..75882a094c 100644 > --- a/block/block-copy.c > +++ b/block/block-copy.c [...] > @@ -646,16 +653,16 @@ out: > * it means that some I/O operation failed in context of _this_ block_copy call, > * not some parallel operation. > */ > -int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes, > - bool *error_is_read) > +static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) > { > int ret; > > do { > - ret = block_copy_dirty_clusters(s, offset, bytes, error_is_read); > + ret = block_copy_dirty_clusters(call_state); It’s possible that much of this code will change in a future patch of this series, but as it is, it might be nice to make block_copy_dirty_clusters’s argument a const pointer so it’s clear that the call to block_copy_wait_one() below will use the original @offset and @bytes values. *shrug* Reviewed-by: Max Reitz <mreitz@redhat.com> > > if (ret == 0) { > - ret = block_copy_wait_one(s, offset, bytes); > + ret = block_copy_wait_one(call_state->s, call_state->offset, > + call_state->bytes); > } > > /*
17.07.2020 16:45, Max Reitz wrote: > On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote: >> Refactor common path to use BlockCopyCallState pointer as parameter, to >> prepare it for use in asynchronous block-copy (at least, we'll need to >> run block-copy in a coroutine, passing the whole parameters as one >> pointer). >> >> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >> --- >> block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------ >> 1 file changed, 38 insertions(+), 13 deletions(-) >> >> diff --git a/block/block-copy.c b/block/block-copy.c >> index 43a018d190..75882a094c 100644 >> --- a/block/block-copy.c >> +++ b/block/block-copy.c > > [...] > >> @@ -646,16 +653,16 @@ out: >> * it means that some I/O operation failed in context of _this_ block_copy call, >> * not some parallel operation. >> */ >> -int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes, >> - bool *error_is_read) >> +static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) >> { >> int ret; >> >> do { >> - ret = block_copy_dirty_clusters(s, offset, bytes, error_is_read); >> + ret = block_copy_dirty_clusters(call_state); > > It’s possible that much of this code will change in a future patch of > this series, but as it is, it might be nice to make > block_copy_dirty_clusters’s argument a const pointer so it’s clear that > the call to block_copy_wait_one() below will use the original @offset > and @bytes values. Hm. I'm trying this, and it doesn't work: block_copy_task_entry() wants to change call_state: t->call_state->failed = true; > > *shrug* > > Reviewed-by: Max Reitz <mreitz@redhat.com> > >> >> if (ret == 0) { >> - ret = block_copy_wait_one(s, offset, bytes); >> + ret = block_copy_wait_one(call_state->s, call_state->offset, >> + call_state->bytes); >> } >> >> /* >
On 18.09.20 22:11, Vladimir Sementsov-Ogievskiy wrote: > 17.07.2020 16:45, Max Reitz wrote: >> On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote: >>> Refactor common path to use BlockCopyCallState pointer as parameter, to >>> prepare it for use in asynchronous block-copy (at least, we'll need to >>> run block-copy in a coroutine, passing the whole parameters as one >>> pointer). >>> >>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> >>> --- >>> block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------ >>> 1 file changed, 38 insertions(+), 13 deletions(-) >>> >>> diff --git a/block/block-copy.c b/block/block-copy.c >>> index 43a018d190..75882a094c 100644 >>> --- a/block/block-copy.c >>> +++ b/block/block-copy.c >> >> [...] >> >>> @@ -646,16 +653,16 @@ out: >>> * it means that some I/O operation failed in context of _this_ >>> block_copy call, >>> * not some parallel operation. >>> */ >>> -int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, >>> int64_t bytes, >>> - bool *error_is_read) >>> +static int coroutine_fn block_copy_common(BlockCopyCallState >>> *call_state) >>> { >>> int ret; >>> do { >>> - ret = block_copy_dirty_clusters(s, offset, bytes, >>> error_is_read); >>> + ret = block_copy_dirty_clusters(call_state); >> >> It’s possible that much of this code will change in a future patch of >> this series, but as it is, it might be nice to make >> block_copy_dirty_clusters’s argument a const pointer so it’s clear that >> the call to block_copy_wait_one() below will use the original @offset >> and @bytes values. > > Hm. I'm trying this, and it doesn't work: > > block_copy_task_entry() wants to change call_state: > > t->call_state->failed = true; Too bad :) >> *shrug* >> >> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >>> if (ret == 0) { >>> - ret = block_copy_wait_one(s, offset, bytes); >>> + ret = block_copy_wait_one(call_state->s, >>> call_state->offset, >>> + call_state->bytes); >>> } >>> /* >> > >
diff --git a/block/block-copy.c b/block/block-copy.c index 43a018d190..75882a094c 100644 --- a/block/block-copy.c +++ b/block/block-copy.c @@ -30,7 +30,15 @@ static coroutine_fn int block_copy_task_entry(AioTask *task); typedef struct BlockCopyCallState { + /* IN parameters */ + BlockCopyState *s; + int64_t offset; + int64_t bytes; + + /* State */ bool failed; + + /* OUT parameters */ bool error_is_read; } BlockCopyCallState; @@ -541,15 +549,17 @@ int64_t block_copy_reset_unallocated(BlockCopyState *s, * Returns 1 if dirty clusters found and successfully copied, 0 if no dirty * clusters found and -errno on failure. */ -static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s, - int64_t offset, int64_t bytes, - bool *error_is_read) +static int coroutine_fn +block_copy_dirty_clusters(BlockCopyCallState *call_state) { + BlockCopyState *s = call_state->s; + int64_t offset = call_state->offset; + int64_t bytes = call_state->bytes; + int ret = 0; bool found_dirty = false; int64_t end = offset + bytes; AioTaskPool *aio = NULL; - BlockCopyCallState call_state = {false, false}; /* * block_copy() user is responsible for keeping source and target in same @@ -565,7 +575,7 @@ static int coroutine_fn block_copy_dirty_clusters(BlockCopyState *s, BlockCopyTask *task; int64_t status_bytes; - task = block_copy_task_create(s, &call_state, offset, bytes); + task = block_copy_task_create(s, call_state, offset, bytes); if (!task) { /* No more dirty bits in the bitmap */ trace_block_copy_skip_range(s, offset, bytes); @@ -630,15 +640,12 @@ out: aio_task_pool_free(aio); } - if (error_is_read && ret < 0) { - *error_is_read = call_state.error_is_read; - } return ret < 0 ? ret : found_dirty; } /* - * block_copy + * block_copy_common * * Copy requested region, accordingly to dirty bitmap. * Collaborate with parallel block_copy requests: if they succeed it will help @@ -646,16 +653,16 @@ out: * it means that some I/O operation failed in context of _this_ block_copy call, * not some parallel operation. */ -int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes, - bool *error_is_read) +static int coroutine_fn block_copy_common(BlockCopyCallState *call_state) { int ret; do { - ret = block_copy_dirty_clusters(s, offset, bytes, error_is_read); + ret = block_copy_dirty_clusters(call_state); if (ret == 0) { - ret = block_copy_wait_one(s, offset, bytes); + ret = block_copy_wait_one(call_state->s, call_state->offset, + call_state->bytes); } /* @@ -672,6 +679,24 @@ int coroutine_fn block_copy(BlockCopyState *s, int64_t offset, int64_t bytes, return ret; } +int coroutine_fn block_copy(BlockCopyState *s, int64_t start, int64_t bytes, + bool *error_is_read) +{ + BlockCopyCallState call_state = { + .s = s, + .offset = start, + .bytes = bytes, + }; + + int ret = block_copy_common(&call_state); + + if (error_is_read && ret < 0) { + *error_is_read = call_state.error_is_read; + } + + return ret; +} + BdrvDirtyBitmap *block_copy_dirty_bitmap(BlockCopyState *s) { return s->copy_bitmap;
Refactor common path to use BlockCopyCallState pointer as parameter, to prepare it for use in asynchronous block-copy (at least, we'll need to run block-copy in a coroutine, passing the whole parameters as one pointer). Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> --- block/block-copy.c | 51 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 38 insertions(+), 13 deletions(-)