diff mbox

[RFC,v2,4/6] replication: fix code logic with the new shared_disk option

Message ID 1480926904-17596-5-git-send-email-zhang.zhanghailiang@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zhanghailiang Dec. 5, 2016, 8:35 a.m. UTC 
Some code logic only be needed in non-shared disk, here
we adjust these codes to prepare for shared disk scenario.

Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
---
 block/replication.c | 47 ++++++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 19 deletions(-)

Comments

Changlong Xie Dec. 20, 2016, 12:42 p.m. UTC | #1 
On 12/05/2016 04:35 PM, zhanghailiang wrote:
> Some code logic only be needed in non-shared disk, here
> we adjust these codes to prepare for shared disk scenario.
>
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>   block/replication.c | 47 ++++++++++++++++++++++++++++-------------------
>   1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/block/replication.c b/block/replication.c
> index 35e9ab3..6574cc2 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -531,21 +531,28 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>               aio_context_release(aio_context);
>               return;
>           }
> -        bdrv_op_block_all(top_bs, s->blocker);
> -        bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
>
> -        job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
> -                                0, MIRROR_SYNC_MODE_NONE, NULL, false,
> +        /*
> +         * Only in the case of non-shared disk,
> +         * the backup job is in the secondary side
> +         */
> +        if (!s->is_shared_disk) {
> +            bdrv_op_block_all(top_bs, s->blocker);
> +            bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
> +            job = backup_job_create(NULL, s->secondary_disk->bs,
> +                                s->hidden_disk->bs, 0,
> +                                MIRROR_SYNC_MODE_NONE, NULL, false,
>                                   BLOCKDEV_ON_ERROR_REPORT,
>                                   BLOCKDEV_ON_ERROR_REPORT, BLOCK_JOB_INTERNAL,
>                                   backup_job_completed, bs, NULL, &local_err);

Coding style here.

> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            backup_job_cleanup(bs);
> -            aio_context_release(aio_context);
> -            return;
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                backup_job_cleanup(bs);
> +                aio_context_release(aio_context);
> +                return;
> +            }
> +            block_job_start(job);
>           }
> -        block_job_start(job);
>
>           secondary_do_checkpoint(s, errp);
>           break;
> @@ -575,14 +582,16 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
>       case REPLICATION_MODE_PRIMARY:
>           break;
>       case REPLICATION_MODE_SECONDARY:
> -        if (!s->secondary_disk->bs->job) {
> -            error_setg(errp, "Backup job was cancelled unexpectedly");
> -            break;
> -        }
> -        backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
> -        if (local_err) {
> -            error_propagate(errp, local_err);
> -            break;
> +        if (!s->is_shared_disk) {
> +            if (!s->secondary_disk->bs->job) {
> +                error_setg(errp, "Backup job was cancelled unexpectedly");
> +                break;
> +            }
> +            backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
> +            if (local_err) {
> +                error_propagate(errp, local_err);
> +                break;
> +            }
>           }
>           secondary_do_checkpoint(s, errp);
>           break;
> @@ -663,7 +672,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
>            * before the BDS is closed, because we will access hidden
>            * disk, secondary disk in backup_job_completed().
>            */
> -        if (s->secondary_disk->bs->job) {
> +        if (!s->is_shared_disk && s->secondary_disk->bs->job) {
>               block_job_cancel_sync(s->secondary_disk->bs->job);
>           }
>
>
Stefan Hajnoczi Jan. 17, 2017, 1:15 p.m. UTC | #2 
On Mon, Dec 05, 2016 at 04:35:02PM +0800, zhanghailiang wrote:
> Some code logic only be needed in non-shared disk, here
> we adjust these codes to prepare for shared disk scenario.
> 
> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
> ---
>  block/replication.c | 47 ++++++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 19 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Zhanghailiang Jan. 18, 2017, 6:53 a.m. UTC | #3 
On 2016/12/20 20:42, Changlong Xie wrote:
> On 12/05/2016 04:35 PM, zhanghailiang wrote:
>> Some code logic only be needed in non-shared disk, here
>> we adjust these codes to prepare for shared disk scenario.
>>
>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>> ---
>>    block/replication.c | 47 ++++++++++++++++++++++++++++-------------------
>>    1 file changed, 28 insertions(+), 19 deletions(-)
>>
>> diff --git a/block/replication.c b/block/replication.c
>> index 35e9ab3..6574cc2 100644
>> --- a/block/replication.c
>> +++ b/block/replication.c
>> @@ -531,21 +531,28 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>>                aio_context_release(aio_context);
>>                return;
>>            }
>> -        bdrv_op_block_all(top_bs, s->blocker);
>> -        bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
>>
>> -        job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
>> -                                0, MIRROR_SYNC_MODE_NONE, NULL, false,
>> +        /*
>> +         * Only in the case of non-shared disk,
>> +         * the backup job is in the secondary side
>> +         */
>> +        if (!s->is_shared_disk) {
>> +            bdrv_op_block_all(top_bs, s->blocker);
>> +            bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
>> +            job = backup_job_create(NULL, s->secondary_disk->bs,
>> +                                s->hidden_disk->bs, 0,
>> +                                MIRROR_SYNC_MODE_NONE, NULL, false,
>>                                    BLOCKDEV_ON_ERROR_REPORT,
>>                                    BLOCKDEV_ON_ERROR_REPORT, BLOCK_JOB_INTERNAL,
>>                                    backup_job_completed, bs, NULL, &local_err);
>
> Coding style here.
>

Will fix it in next version, thanks.

>> -        if (local_err) {
>> -            error_propagate(errp, local_err);
>> -            backup_job_cleanup(bs);
>> -            aio_context_release(aio_context);
>> -            return;
>> +            if (local_err) {
>> +                error_propagate(errp, local_err);
>> +                backup_job_cleanup(bs);
>> +                aio_context_release(aio_context);
>> +                return;
>> +            }
>> +            block_job_start(job);
>>            }
>> -        block_job_start(job);
>>
>>            secondary_do_checkpoint(s, errp);
>>            break;
>> @@ -575,14 +582,16 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
>>        case REPLICATION_MODE_PRIMARY:
>>            break;
>>        case REPLICATION_MODE_SECONDARY:
>> -        if (!s->secondary_disk->bs->job) {
>> -            error_setg(errp, "Backup job was cancelled unexpectedly");
>> -            break;
>> -        }
>> -        backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
>> -        if (local_err) {
>> -            error_propagate(errp, local_err);
>> -            break;
>> +        if (!s->is_shared_disk) {
>> +            if (!s->secondary_disk->bs->job) {
>> +                error_setg(errp, "Backup job was cancelled unexpectedly");
>> +                break;
>> +            }
>> +            backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
>> +            if (local_err) {
>> +                error_propagate(errp, local_err);
>> +                break;
>> +            }
>>            }
>>            secondary_do_checkpoint(s, errp);
>>            break;
>> @@ -663,7 +672,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
>>             * before the BDS is closed, because we will access hidden
>>             * disk, secondary disk in backup_job_completed().
>>             */
>> -        if (s->secondary_disk->bs->job) {
>> +        if (!s->is_shared_disk && s->secondary_disk->bs->job) {
>>                block_job_cancel_sync(s->secondary_disk->bs->job);
>>            }
>>
>>
>
>
>
> .
>
diff mbox

Patch

diff --git a/block/replication.c b/block/replication.c
index 35e9ab3..6574cc2 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -531,21 +531,28 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
             aio_context_release(aio_context);
             return;
         }
-        bdrv_op_block_all(top_bs, s->blocker);
-        bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
 
-        job = backup_job_create(NULL, s->secondary_disk->bs, s->hidden_disk->bs,
-                                0, MIRROR_SYNC_MODE_NONE, NULL, false,
+        /*
+         * Only in the case of non-shared disk,
+         * the backup job is in the secondary side
+         */
+        if (!s->is_shared_disk) {
+            bdrv_op_block_all(top_bs, s->blocker);
+            bdrv_op_unblock(top_bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker);
+            job = backup_job_create(NULL, s->secondary_disk->bs,
+                                s->hidden_disk->bs, 0,
+                                MIRROR_SYNC_MODE_NONE, NULL, false,
                                 BLOCKDEV_ON_ERROR_REPORT,
                                 BLOCKDEV_ON_ERROR_REPORT, BLOCK_JOB_INTERNAL,
                                 backup_job_completed, bs, NULL, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            backup_job_cleanup(bs);
-            aio_context_release(aio_context);
-            return;
+            if (local_err) {
+                error_propagate(errp, local_err);
+                backup_job_cleanup(bs);
+                aio_context_release(aio_context);
+                return;
+            }
+            block_job_start(job);
         }
-        block_job_start(job);
 
         secondary_do_checkpoint(s, errp);
         break;
@@ -575,14 +582,16 @@  static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
     case REPLICATION_MODE_PRIMARY:
         break;
     case REPLICATION_MODE_SECONDARY:
-        if (!s->secondary_disk->bs->job) {
-            error_setg(errp, "Backup job was cancelled unexpectedly");
-            break;
-        }
-        backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
-        if (local_err) {
-            error_propagate(errp, local_err);
-            break;
+        if (!s->is_shared_disk) {
+            if (!s->secondary_disk->bs->job) {
+                error_setg(errp, "Backup job was cancelled unexpectedly");
+                break;
+            }
+            backup_do_checkpoint(s->secondary_disk->bs->job, &local_err);
+            if (local_err) {
+                error_propagate(errp, local_err);
+                break;
+            }
         }
         secondary_do_checkpoint(s, errp);
         break;
@@ -663,7 +672,7 @@  static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
          * before the BDS is closed, because we will access hidden
          * disk, secondary disk in backup_job_completed().
          */
-        if (s->secondary_disk->bs->job) {
+        if (!s->is_shared_disk && s->secondary_disk->bs->job) {
             block_job_cancel_sync(s->secondary_disk->bs->job);
         }