Re: [Xen-devel] [PATCH RFC] remus: implement remus replicated checkpointing disk

[Ian Jackson <Ian.Jackson@xxxxxxxxxxxxx>]

Lai Jiangshan writes ("Re: [PATCH RFC] remus: implement remus replicated 
checkpointing disk"):
> On 03/10/2014 07:28 PM, Ian Jackson wrote:
> > I'm not sure I understand how this can be true.  Can you point me at
> > an explanation of the supposed semantics of the remus disk commit ?
> > (Eg in a remus design document or even a paper.)  I suspect something
> > ought to be done here.
> 
> in drbd-remus case, DRBD_SEND_CHECKPOINT(drbd_postsuspend()) will
> do the committing works asynchronously. 

Um, I'm still not convinced that this is right.  Can you point me
to the relevant design documentation for remus (as I say above) and
the relevant documentation for the drbd checkpoint facility ?  That
will allow me to understand this and check that it's correct.

> >> +    for (i = 0; i < nr_disks; i++) {
> >> +        remus_disk = NULL;
> >> +        for (j = 0; j < ARRAY_SIZE(remus_disk_types); j++) {
> >> +            type = remus_disk_types[j];
> >> +            remus_disk = type->setup(gc, &disks[i]);
> >> +            if (!remus_disk)
> >> +                break;
> >> +
> >> +            remus_state->disks[i] = remus_disk;
> >> +            remus_disk->disk = &disks[i];
> >> +            remus_disk->type = type;
> >> +        }
> > 
> > I think this code is wrong.  It appears to call all of the setup
> > functions, not just one, and overwrite remus_disk with their
> > successive results.
> 
> If the user use remus disk replication, it is required that
> all the disks should support remus disk replication.

Oh, I see.

> So we call setup to all disk. If any disk doesn't support remus
> or any disk fail to setup, this libxl__remus_disks_setup() should failed too.

Right.  I think this deserves a long message.  And in that case,
I think:

+        if (!remus_disk) {
+            remus_state->nr_disks = i;
+            libxl__remus_disks_teardown(remus_state);
+            return -1;
+        }

Instead of rewinding nr_disks, it would be better to make

+void libxl__remus_disks_teardown(libxl__remus_state *state)
+{
+    int i;
+
+    for (i = 0; i < state->nr_disks; i++)
+        state->disks[i]->type->teardown(state->disks[i]);

this code not mind if disks[i] == NULL;

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel