[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-block] [Qemu-devel] [PATCH v5 06/16] block/mirror: conservativ
From: |
John Snow |
Subject: |
Re: [Qemu-block] [Qemu-devel] [PATCH v5 06/16] block/mirror: conservative mirror_exit refactor |
Date: |
Thu, 6 Sep 2018 16:31:47 -0400 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.8.0 |
On 09/06/2018 12:57 PM, Jeff Cody wrote:
> On Thu, Sep 06, 2018 at 09:02:15AM -0400, John Snow wrote:
>> For purposes of minimum code movement, refactor the mirror_exit
>> callback to use the post-finalization callbacks in a trivial way.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>> block/mirror.c | 39 ++++++++++++++++++++++++++++-----------
>> 1 file changed, 28 insertions(+), 11 deletions(-)
>>
>> diff --git a/block/mirror.c b/block/mirror.c
>> index bd3e908710..a92b4702c5 100644
>> --- a/block/mirror.c
>> +++ b/block/mirror.c
>> @@ -79,6 +79,7 @@ typedef struct MirrorBlockJob {
>> int max_iov;
>> bool initial_zeroing_ongoing;
>> int in_active_write_counter;
>> + bool prepared;
>> } MirrorBlockJob;
>>
>> typedef struct MirrorBDSOpaque {
>> @@ -607,7 +608,7 @@ static void mirror_wait_for_all_io(MirrorBlockJob *s)
>> }
>> }
>>
>> -static void mirror_exit(Job *job)
/**
* mirror_exit_common: handle both abort() and prepare() cases.
* for .prepare, returns 0 on success and -errno on failure.
* for .abort cases, denoted by abort = true, MUST return 0.
*/
>> +static int mirror_exit_common(Job *job)
>> {
>> MirrorBlockJob *s = container_of(job, MirrorBlockJob, common.job);
>> BlockJob *bjob = &s->common;
>> @@ -617,7 +618,13 @@ static void mirror_exit(Job *job)
>> BlockDriverState *target_bs = blk_bs(s->target);
>> BlockDriverState *mirror_top_bs = s->mirror_top_bs;
>> Error *local_err = NULL;
>> - int ret = job->ret;
>> + bool abort = job->ret < 0;
>> + int ret = 0;
>> +
>> + if (s->prepared) {
>> + return 0;
>> + }
>> + s->prepared = true;
>>
>> bdrv_release_dirty_bitmap(src, s->dirty_bitmap);
>>
>> @@ -642,7 +649,7 @@ static void mirror_exit(Job *job)
>> * required before it could become a backing file of target_bs. */
>> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
>> &error_abort);
>> - if (ret == 0 && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>> + if (!abort && s->backing_mode == MIRROR_SOURCE_BACKING_CHAIN) {
>> BlockDriverState *backing = s->is_none_mode ? src : s->base;
>> if (backing_bs(target_bs) != backing) {
>> bdrv_set_backing_hd(target_bs, backing, &local_err);
>> @@ -658,11 +665,8 @@ static void mirror_exit(Job *job)
>> aio_context_acquire(replace_aio_context);
>> }
>>
>> - if (s->should_complete && ret == 0) {
>> - BlockDriverState *to_replace = src;
>> - if (s->to_replace) {
>> - to_replace = s->to_replace;
>> - }
>> + if (s->should_complete && !abort) {
>> + BlockDriverState *to_replace = s->to_replace ?: src;
>>
>> if (bdrv_get_flags(target_bs) != bdrv_get_flags(to_replace)) {
>> bdrv_reopen(target_bs, bdrv_get_flags(to_replace), NULL);
>> @@ -711,7 +715,18 @@ static void mirror_exit(Job *job)
>> bdrv_unref(mirror_top_bs);
>> bdrv_unref(src);
>>
>> - job->ret = ret;
>> + return ret;
>> +}
>> +
>> +static int mirror_prepare(Job *job)
>> +{
>> + return mirror_exit_common(job);
>> +}
>> +
>> +static void mirror_abort(Job *job)
>> +{
>> + int ret = mirror_exit_common(job);
>> + assert(ret == 0);
>
> Not something to hold the series up, but in case a v6 is called for due to
> other changes: I think it may be worth a comment in mirror_exit_common()
> that if abort is true, and we don't return success, QEMU will hit an assert.
> Mainly to prevent someone from including a call with a potential error
> return in the abort path in the future.
>
> Reviewed-by: Jeff Cody <address@hidden>
>
Yeah, I should have added a comment, like the one you read before you
got to this comment, by the very nature of how email works.
--js
- [Qemu-block] [PATCH v5 00/16] jobs: Job Exit Refactoring Pt 2, John Snow, 2018/09/06
- [Qemu-block] [PATCH v5 03/16] block/stream: add block job creation flags, John Snow, 2018/09/06
- [Qemu-block] [PATCH v5 05/16] block/mirror: don't install backing chain on abort, John Snow, 2018/09/06
- [Qemu-block] [PATCH v5 02/16] block/mirror: add block job creation flags, John Snow, 2018/09/06
- [Qemu-block] [PATCH v5 10/16] tests/test-blockjob-txn: move .exit to .clean, John Snow, 2018/09/06
- [Qemu-block] [PATCH v5 14/16] qapi/block-stream: expose new job properties, John Snow, 2018/09/06