diff mbox series

[v3,1/4] replication: Remove s->active_disk

Message ID dd6c3cd4bc77138918c31d590569bdf77b7cef0b.1625680555.git.lukasstraub2@web.de (mailing list archive)
State New, archived
Headers show
Series replication: Properly attach children | expand

Commit Message

Lukas Straub July 7, 2021, 6:15 p.m. UTC 
s->active_disk is bs->file. Remove it and use local variables instead.

Signed-off-by: Lukas Straub <lukasstraub2@web.de>
---
 block/replication.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

--
2.20.1

Comments

Vladimir Sementsov-Ogievskiy July 9, 2021, 7:11 a.m. UTC | #1 
07.07.2021 21:15, Lukas Straub wrote:
> s->active_disk is bs->file. Remove it and use local variables instead.
> 
> Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> ---
>   block/replication.c | 38 +++++++++++++++++++++-----------------
>   1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/block/replication.c b/block/replication.c
> index 52163f2d1f..50940fbe33 100644
> --- a/block/replication.c
> +++ b/block/replication.c
> @@ -35,7 +35,6 @@ typedef enum {
>   typedef struct BDRVReplicationState {
>       ReplicationMode mode;
>       ReplicationStage stage;
> -    BdrvChild *active_disk;
>       BlockJob *commit_job;
>       BdrvChild *hidden_disk;
>       BdrvChild *secondary_disk;
> @@ -307,11 +306,15 @@ out:
>       return ret;
>   }
> 
> -static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
> +static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
>   {
> +    BDRVReplicationState *s = bs->opaque;
> +    BdrvChild *active_disk;

Why not to combine initialization into definition:

BdrvChild *active_disk = bs->file;

>       Error *local_err = NULL;
>       int ret;
> 
> +    active_disk = bs->file;
> +
>       if (!s->backup_job) {
>           error_setg(errp, "Backup job was cancelled unexpectedly");
>           return;
> @@ -323,13 +326,13 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
>           return;
>       }
> 
> -    if (!s->active_disk->bs->drv) {
> +    if (!active_disk->bs->drv) {
>           error_setg(errp, "Active disk %s is ejected",
> -                   s->active_disk->bs->node_name);
> +                   active_disk->bs->node_name);
>           return;
>       }
> 
> -    ret = bdrv_make_empty(s->active_disk, errp);
> +    ret = bdrv_make_empty(active_disk, errp);
>       if (ret < 0) {
>           return;
>       }
> @@ -451,6 +454,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>       BlockDriverState *bs = rs->opaque;
>       BDRVReplicationState *s;
>       BlockDriverState *top_bs;
> +    BdrvChild *active_disk;
>       int64_t active_length, hidden_length, disk_length;
>       AioContext *aio_context;
>       Error *local_err = NULL;
> @@ -488,15 +492,14 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>       case REPLICATION_MODE_PRIMARY:
>           break;
>       case REPLICATION_MODE_SECONDARY:
> -        s->active_disk = bs->file;
> -        if (!s->active_disk || !s->active_disk->bs ||
> -                                    !s->active_disk->bs->backing) {
> +        active_disk = bs->file;

Here initializing active_disk only here makes sense: we consider "active disk" only in secondary mode. Right?

> +        if (!active_disk || !active_disk->bs || !active_disk->bs->backing) {
>               error_setg(errp, "Active disk doesn't have backing file");
>               aio_context_release(aio_context);
>               return;
>           }
> 
> -        s->hidden_disk = s->active_disk->bs->backing;
> +        s->hidden_disk = active_disk->bs->backing;
>           if (!s->hidden_disk->bs || !s->hidden_disk->bs->backing) {
>               error_setg(errp, "Hidden disk doesn't have backing file");
>               aio_context_release(aio_context);
> @@ -511,7 +514,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>           }
> 
>           /* verify the length */
> -        active_length = bdrv_getlength(s->active_disk->bs);
> +        active_length = bdrv_getlength(active_disk->bs);
>           hidden_length = bdrv_getlength(s->hidden_disk->bs);
>           disk_length = bdrv_getlength(s->secondary_disk->bs);
>           if (active_length < 0 || hidden_length < 0 || disk_length < 0 ||
> @@ -523,9 +526,9 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>           }
> 
>           /* Must be true, or the bdrv_getlength() calls would have failed */
> -        assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
> +        assert(active_disk->bs->drv && s->hidden_disk->bs->drv);
> 
> -        if (!s->active_disk->bs->drv->bdrv_make_empty ||
> +        if (!active_disk->bs->drv->bdrv_make_empty ||
>               !s->hidden_disk->bs->drv->bdrv_make_empty) {
>               error_setg(errp,
>                          "Active disk or hidden disk doesn't support make_empty");
> @@ -579,7 +582,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
>       s->stage = BLOCK_REPLICATION_RUNNING;
> 
>       if (s->mode == REPLICATION_MODE_SECONDARY) {
> -        secondary_do_checkpoint(s, errp);
> +        secondary_do_checkpoint(bs, errp);
>       }
> 
>       s->error = 0;
> @@ -608,7 +611,7 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
>       }
> 
>       if (s->mode == REPLICATION_MODE_SECONDARY) {
> -        secondary_do_checkpoint(s, errp);
> +        secondary_do_checkpoint(bs, errp);
>       }
>       aio_context_release(aio_context);
>   }
> @@ -645,7 +648,6 @@ static void replication_done(void *opaque, int ret)
>       if (ret == 0) {
>           s->stage = BLOCK_REPLICATION_DONE;
> 
> -        s->active_disk = NULL;
>           s->secondary_disk = NULL;
>           s->hidden_disk = NULL;
>           s->error = 0;
> @@ -659,11 +661,13 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
>   {
>       BlockDriverState *bs = rs->opaque;
>       BDRVReplicationState *s;
> +    BdrvChild *active_disk;
>       AioContext *aio_context;
> 
>       aio_context = bdrv_get_aio_context(bs);
>       aio_context_acquire(aio_context);
>       s = bs->opaque;
> +    active_disk = bs->file;
> 
>       if (s->stage == BLOCK_REPLICATION_DONE ||
>           s->stage == BLOCK_REPLICATION_FAILOVER) {
> @@ -698,7 +702,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
>           }
> 
>           if (!failover) {
> -            secondary_do_checkpoint(s, errp);
> +            secondary_do_checkpoint(bs, errp);
>               s->stage = BLOCK_REPLICATION_DONE;
>               aio_context_release(aio_context);
>               return;
> @@ -706,7 +710,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
> 
>           s->stage = BLOCK_REPLICATION_FAILOVER;

For consistency, it seems right to initialize active_disk only in "case REPLICATION_MODE_SECONDARY:", like above..

But then, it becomes obvious that no sense in creating additional variable to use it once.. So here I'd just use bs->file->bs

>           s->commit_job = commit_active_start(
> -                            NULL, s->active_disk->bs, s->secondary_disk->bs,
> +                            NULL, active_disk->bs, s->secondary_disk->bs,
>                               JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
>                               NULL, replication_done, bs, true, errp);
>           break;
> --
> 2.20.1
> 


Anyway:
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Lukas Straub July 9, 2021, 12:11 p.m. UTC | #2 
On Fri, 9 Jul 2021 10:11:15 +0300
Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> wrote:

> 07.07.2021 21:15, Lukas Straub wrote:
> > s->active_disk is bs->file. Remove it and use local variables instead.
> > 
> > Signed-off-by: Lukas Straub <lukasstraub2@web.de>
> > ---
> >   block/replication.c | 38 +++++++++++++++++++++-----------------
> >   1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/block/replication.c b/block/replication.c
> > index 52163f2d1f..50940fbe33 100644
> > --- a/block/replication.c
> > +++ b/block/replication.c
> > @@ -35,7 +35,6 @@ typedef enum {
> >   typedef struct BDRVReplicationState {
> >       ReplicationMode mode;
> >       ReplicationStage stage;
> > -    BdrvChild *active_disk;
> >       BlockJob *commit_job;
> >       BdrvChild *hidden_disk;
> >       BdrvChild *secondary_disk;
> > @@ -307,11 +306,15 @@ out:
> >       return ret;
> >   }
> > 
> > -static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
> > +static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
> >   {
> > +    BDRVReplicationState *s = bs->opaque;
> > +    BdrvChild *active_disk;  
> 
> Why not to combine initialization into definition:
> 
> BdrvChild *active_disk = bs->file;

Ok, will fix.

> >       Error *local_err = NULL;
> >       int ret;
> > 
> > +    active_disk = bs->file;
> > +
> >       if (!s->backup_job) {
> >           error_setg(errp, "Backup job was cancelled unexpectedly");
> >           return;
> > @@ -323,13 +326,13 @@ static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
> >           return;
> >       }
> > 
> > -    if (!s->active_disk->bs->drv) {
> > +    if (!active_disk->bs->drv) {
> >           error_setg(errp, "Active disk %s is ejected",
> > -                   s->active_disk->bs->node_name);
> > +                   active_disk->bs->node_name);
> >           return;
> >       }
> > 
> > -    ret = bdrv_make_empty(s->active_disk, errp);
> > +    ret = bdrv_make_empty(active_disk, errp);
> >       if (ret < 0) {
> >           return;
> >       }
> > @@ -451,6 +454,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
> >       BlockDriverState *bs = rs->opaque;
> >       BDRVReplicationState *s;
> >       BlockDriverState *top_bs;
> > +    BdrvChild *active_disk;
> >       int64_t active_length, hidden_length, disk_length;
> >       AioContext *aio_context;
> >       Error *local_err = NULL;
> > @@ -488,15 +492,14 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
> >       case REPLICATION_MODE_PRIMARY:
> >           break;
> >       case REPLICATION_MODE_SECONDARY:
> > -        s->active_disk = bs->file;
> > -        if (!s->active_disk || !s->active_disk->bs ||
> > -                                    !s->active_disk->bs->backing) {
> > +        active_disk = bs->file;  
> 
> Here initializing active_disk only here makes sense: we consider "active disk" only in secondary mode. Right?

Yes.

> > +        if (!active_disk || !active_disk->bs || !active_disk->bs->backing) {
> >               error_setg(errp, "Active disk doesn't have backing file");
> >               aio_context_release(aio_context);
> >               return;
> >           }
> > 
> > -        s->hidden_disk = s->active_disk->bs->backing;
> > +        s->hidden_disk = active_disk->bs->backing;
> >           if (!s->hidden_disk->bs || !s->hidden_disk->bs->backing) {
> >               error_setg(errp, "Hidden disk doesn't have backing file");
> >               aio_context_release(aio_context);
> > @@ -511,7 +514,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
> >           }
> > 
> >           /* verify the length */
> > -        active_length = bdrv_getlength(s->active_disk->bs);
> > +        active_length = bdrv_getlength(active_disk->bs);
> >           hidden_length = bdrv_getlength(s->hidden_disk->bs);
> >           disk_length = bdrv_getlength(s->secondary_disk->bs);
> >           if (active_length < 0 || hidden_length < 0 || disk_length < 0 ||
> > @@ -523,9 +526,9 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
> >           }
> > 
> >           /* Must be true, or the bdrv_getlength() calls would have failed */
> > -        assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
> > +        assert(active_disk->bs->drv && s->hidden_disk->bs->drv);
> > 
> > -        if (!s->active_disk->bs->drv->bdrv_make_empty ||
> > +        if (!active_disk->bs->drv->bdrv_make_empty ||
> >               !s->hidden_disk->bs->drv->bdrv_make_empty) {
> >               error_setg(errp,
> >                          "Active disk or hidden disk doesn't support make_empty");
> > @@ -579,7 +582,7 @@ static void replication_start(ReplicationState *rs, ReplicationMode mode,
> >       s->stage = BLOCK_REPLICATION_RUNNING;
> > 
> >       if (s->mode == REPLICATION_MODE_SECONDARY) {
> > -        secondary_do_checkpoint(s, errp);
> > +        secondary_do_checkpoint(bs, errp);
> >       }
> > 
> >       s->error = 0;
> > @@ -608,7 +611,7 @@ static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
> >       }
> > 
> >       if (s->mode == REPLICATION_MODE_SECONDARY) {
> > -        secondary_do_checkpoint(s, errp);
> > +        secondary_do_checkpoint(bs, errp);
> >       }
> >       aio_context_release(aio_context);
> >   }
> > @@ -645,7 +648,6 @@ static void replication_done(void *opaque, int ret)
> >       if (ret == 0) {
> >           s->stage = BLOCK_REPLICATION_DONE;
> > 
> > -        s->active_disk = NULL;
> >           s->secondary_disk = NULL;
> >           s->hidden_disk = NULL;
> >           s->error = 0;
> > @@ -659,11 +661,13 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
> >   {
> >       BlockDriverState *bs = rs->opaque;
> >       BDRVReplicationState *s;
> > +    BdrvChild *active_disk;
> >       AioContext *aio_context;
> > 
> >       aio_context = bdrv_get_aio_context(bs);
> >       aio_context_acquire(aio_context);
> >       s = bs->opaque;
> > +    active_disk = bs->file;
> > 
> >       if (s->stage == BLOCK_REPLICATION_DONE ||
> >           s->stage == BLOCK_REPLICATION_FAILOVER) {
> > @@ -698,7 +702,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
> >           }
> > 
> >           if (!failover) {
> > -            secondary_do_checkpoint(s, errp);
> > +            secondary_do_checkpoint(bs, errp);
> >               s->stage = BLOCK_REPLICATION_DONE;
> >               aio_context_release(aio_context);
> >               return;
> > @@ -706,7 +710,7 @@ static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
> > 
> >           s->stage = BLOCK_REPLICATION_FAILOVER;  
> 
> For consistency, it seems right to initialize active_disk only in "case REPLICATION_MODE_SECONDARY:", like above..
> 
> But then, it becomes obvious that no sense in creating additional variable to use it once.. So here I'd just use bs->file->bs

Ok, will fix.

> >           s->commit_job = commit_active_start(
> > -                            NULL, s->active_disk->bs, s->secondary_disk->bs,
> > +                            NULL, active_disk->bs, s->secondary_disk->bs,
> >                               JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
> >                               NULL, replication_done, bs, true, errp);
> >           break;
> > --
> > 2.20.1
> >   
> 
> 
> Anyway:
> Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> 

Thanks,
Lukas Straub

--
diff mbox series

Patch

diff --git a/block/replication.c b/block/replication.c
index 52163f2d1f..50940fbe33 100644
--- a/block/replication.c
+++ b/block/replication.c
@@ -35,7 +35,6 @@  typedef enum {
 typedef struct BDRVReplicationState {
     ReplicationMode mode;
     ReplicationStage stage;
-    BdrvChild *active_disk;
     BlockJob *commit_job;
     BdrvChild *hidden_disk;
     BdrvChild *secondary_disk;
@@ -307,11 +306,15 @@  out:
     return ret;
 }

-static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
+static void secondary_do_checkpoint(BlockDriverState *bs, Error **errp)
 {
+    BDRVReplicationState *s = bs->opaque;
+    BdrvChild *active_disk;
     Error *local_err = NULL;
     int ret;

+    active_disk = bs->file;
+
     if (!s->backup_job) {
         error_setg(errp, "Backup job was cancelled unexpectedly");
         return;
@@ -323,13 +326,13 @@  static void secondary_do_checkpoint(BDRVReplicationState *s, Error **errp)
         return;
     }

-    if (!s->active_disk->bs->drv) {
+    if (!active_disk->bs->drv) {
         error_setg(errp, "Active disk %s is ejected",
-                   s->active_disk->bs->node_name);
+                   active_disk->bs->node_name);
         return;
     }

-    ret = bdrv_make_empty(s->active_disk, errp);
+    ret = bdrv_make_empty(active_disk, errp);
     if (ret < 0) {
         return;
     }
@@ -451,6 +454,7 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
     BlockDriverState *bs = rs->opaque;
     BDRVReplicationState *s;
     BlockDriverState *top_bs;
+    BdrvChild *active_disk;
     int64_t active_length, hidden_length, disk_length;
     AioContext *aio_context;
     Error *local_err = NULL;
@@ -488,15 +492,14 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
     case REPLICATION_MODE_PRIMARY:
         break;
     case REPLICATION_MODE_SECONDARY:
-        s->active_disk = bs->file;
-        if (!s->active_disk || !s->active_disk->bs ||
-                                    !s->active_disk->bs->backing) {
+        active_disk = bs->file;
+        if (!active_disk || !active_disk->bs || !active_disk->bs->backing) {
             error_setg(errp, "Active disk doesn't have backing file");
             aio_context_release(aio_context);
             return;
         }

-        s->hidden_disk = s->active_disk->bs->backing;
+        s->hidden_disk = active_disk->bs->backing;
         if (!s->hidden_disk->bs || !s->hidden_disk->bs->backing) {
             error_setg(errp, "Hidden disk doesn't have backing file");
             aio_context_release(aio_context);
@@ -511,7 +514,7 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
         }

         /* verify the length */
-        active_length = bdrv_getlength(s->active_disk->bs);
+        active_length = bdrv_getlength(active_disk->bs);
         hidden_length = bdrv_getlength(s->hidden_disk->bs);
         disk_length = bdrv_getlength(s->secondary_disk->bs);
         if (active_length < 0 || hidden_length < 0 || disk_length < 0 ||
@@ -523,9 +526,9 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
         }

         /* Must be true, or the bdrv_getlength() calls would have failed */
-        assert(s->active_disk->bs->drv && s->hidden_disk->bs->drv);
+        assert(active_disk->bs->drv && s->hidden_disk->bs->drv);

-        if (!s->active_disk->bs->drv->bdrv_make_empty ||
+        if (!active_disk->bs->drv->bdrv_make_empty ||
             !s->hidden_disk->bs->drv->bdrv_make_empty) {
             error_setg(errp,
                        "Active disk or hidden disk doesn't support make_empty");
@@ -579,7 +582,7 @@  static void replication_start(ReplicationState *rs, ReplicationMode mode,
     s->stage = BLOCK_REPLICATION_RUNNING;

     if (s->mode == REPLICATION_MODE_SECONDARY) {
-        secondary_do_checkpoint(s, errp);
+        secondary_do_checkpoint(bs, errp);
     }

     s->error = 0;
@@ -608,7 +611,7 @@  static void replication_do_checkpoint(ReplicationState *rs, Error **errp)
     }

     if (s->mode == REPLICATION_MODE_SECONDARY) {
-        secondary_do_checkpoint(s, errp);
+        secondary_do_checkpoint(bs, errp);
     }
     aio_context_release(aio_context);
 }
@@ -645,7 +648,6 @@  static void replication_done(void *opaque, int ret)
     if (ret == 0) {
         s->stage = BLOCK_REPLICATION_DONE;

-        s->active_disk = NULL;
         s->secondary_disk = NULL;
         s->hidden_disk = NULL;
         s->error = 0;
@@ -659,11 +661,13 @@  static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
 {
     BlockDriverState *bs = rs->opaque;
     BDRVReplicationState *s;
+    BdrvChild *active_disk;
     AioContext *aio_context;

     aio_context = bdrv_get_aio_context(bs);
     aio_context_acquire(aio_context);
     s = bs->opaque;
+    active_disk = bs->file;

     if (s->stage == BLOCK_REPLICATION_DONE ||
         s->stage == BLOCK_REPLICATION_FAILOVER) {
@@ -698,7 +702,7 @@  static void replication_stop(ReplicationState *rs, bool failover, Error **errp)
         }

         if (!failover) {
-            secondary_do_checkpoint(s, errp);
+            secondary_do_checkpoint(bs, errp);
             s->stage = BLOCK_REPLICATION_DONE;
             aio_context_release(aio_context);
             return;
@@ -706,7 +710,7 @@  static void replication_stop(ReplicationState *rs, bool failover, Error **errp)

         s->stage = BLOCK_REPLICATION_FAILOVER;
         s->commit_job = commit_active_start(
-                            NULL, s->active_disk->bs, s->secondary_disk->bs,
+                            NULL, active_disk->bs, s->secondary_disk->bs,
                             JOB_INTERNAL, 0, BLOCKDEV_ON_ERROR_REPORT,
                             NULL, replication_done, bs, true, errp);
         break;