Re: [PATCH v4 3/5] libxl: add support for vscsi

[Olaf Hering <olaf@aepfle.de>]

On Tue, May 05, Wei Liu wrote:

> On Fri, Apr 17, 2015 at 08:30:58AM +0000, Olaf Hering wrote:
> > +'pdev' describes the physical device, preferable in a persistant format.
> "persistent", and please explain when "persistent format" is.

Done.

> > +The backend driver in the pvops kernel is part of the Linux-IO Target framework
> > +(LIO). As such the SCSI devices have to be configured first with the tools
> > +provided by this framework, such as a xen-scsiback aware targetcli. The "pdev"
> What is "targetcli"?

A tool to setup the various backend/frontend drivers of the Linux target
IO framework. http://linux-iscsi.org/wiki/Targetcli
In fact the underlying python-rtslib has to be aware of xen-scsiback.

> > +The dom0 device in either /dev/scsidev or pHOST:CHANNEL:TARGET:LUN notation.
> What is vHOST? What is pHOST?

This refers to the "physical" SCSI host in the backend domain, and vHOST
is the "index number" of a SCSI host presented to the guest.

> > +Its recommended to use persistant names "/dev/disk/by-*/*" to refer to a "pdev".
> "It's"  "persistent"

Done.

> > +    path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> > +    val = libxl__xs_read(gc, t, path);
> > +    LOG(DEBUG, "%s is %s", path, val);
> > +    if (val && strcmp(val, GCSPRINTF("%d", dev_wait)) == 0) {
> > +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/state", be_path, v->vscsi_dev_id);
> > +        xs_rm(ctx->xsh, t, path);
> > +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-devname", be_path, v->vscsi_dev_id);
> > +        xs_rm(ctx->xsh, t, path);
> > +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/p-dev", be_path, v->vscsi_dev_id);
> > +        xs_rm(ctx->xsh, t, path);
> > +        path = GCSPRINTF("%s/vscsi-devs/dev-%u/v-dev", be_path, v->vscsi_dev_id);
> > +        xs_rm(ctx->xsh, t, path);
> > +        path = GCSPRINTF("%s/vscsi-devs/dev-%u", be_path, v->vscsi_dev_id);
> > +        xs_rm(ctx->xsh, t, path);
> Some lines are too long. Please wrap them properly.
> But can you not simplify this by removing be_path all together?

Done.

> Also this function should return error number -- you probably don't want
> caller to commence should things go wrong here.

Why would anyone care about failed xs_rm? The state was forwarded and
the domU is in unknown state if anything happens here. Also that
vscsi-devs/dev-N path will not be used again.

> > +    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/p-devname", v->vscsi_dev_id),
> > +                          v->pdev.p_devname);
> > +    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/v-dev", v->vscsi_dev_id),
> > +                          GCSPRINTF("%u:%u:%u:%u", v->vdev.hst, v->vdev.chn, v->vdev.tgt, v->vdev.lun));
> > +    flexarray_append_pair(back, GCSPRINTF("vscsi-devs/dev-%u/state", v->vscsi_dev_id),
> > +                          GCSPRINTF("%d", XenbusStateInitialising));
> Please fix the lines that are too long.

I did adjustments, but getting below 80 makes it look broken.
I'm willing to donate wider editor windows.

> > +    flexarray_append_pair(back, "frontend-id", GCSPRINTF("%d", aodev->dev->domid));
> > +    flexarray_append_pair(back, "online", "1");
> > +    flexarray_append_pair(back, "state", GCSPRINTF("%d", XenbusStateInitialising));
> > +    flexarray_append_pair(back, "feature-host",
> > +                          libxl_defbool_val(vscsi->feature_host) ?
> > +                          "1" : "0");
> > +
> > +    flexarray_append_pair(front, "backend-id", GCSPRINTF("%d", vscsi->backend_domid));
> > +    flexarray_append_pair(front, "state", GCSPRINTF("%d", XenbusStateInitialising));
> Lines too long.

Getting below 80 makes it look broken.

> > +        /* Append new device or trigger removal */
> > +        for (i = 0; i < vscsi->num_vscsi_devs; i++) {
> > +            v = vscsi->vscsi_devs + i;
> > +            unsigned int nb = 0;
> Move this line at the beginning of this block. I think you will hit a
> gcc warning here, won't you?

No warning, -Werror would have caught this. I think -std=gnu99 allows
this. I have flipped these two lines.

> > +    /* In case of vscsi the copy remains identical to the provided input */
> This comment is confusing.

What is confusing about it? "*vscsi == vscsi_saved". I will remove the
comment.

> > +    be_path = libxl__device_backend_path(gc, aodev->dev);
> > +    if (libxl__xs_directory(gc, XBT_NULL, be_path, &be_dirs)) {
> This looks bogus. Shouldn't you use a real transaction here and in the
> following two functions?

Not sure if that can be racy. Should the for-loop to retry the
transaction be in this function? After all that thing is protected by
the userdata lock.

> > +        rc = libxl__device_vscsi_reconfigure(egc, aodev, vscsi, &d_config, be_path, NULL);
> > +        if (rc)
> > +            goto out;
> > +        /* Notify that this is done */
> > +        aodev->callback(egc, aodev);
> And this is for? You can just do this in out path, right?

This has to be called unconditionally, the outpath does it only in case
of failure.

> > +    } else {
> > +        rc = libxl__device_vscsi_new_backend(egc, aodev, vscsi, &d_config);
> > +        if (rc)
> > +            goto out;
> > +    }
> > +
> > +    rc = 0;
> > +out:
> > +    if (lock) libxl__unlock_domain_userdata(lock);
> > +    libxl_device_vscsi_dispose(&vscsi_saved);
> > +    libxl_domain_config_dispose(&d_config);
> > +    aodev->rc = rc;
> > +    if (rc) aodev->callback(egc, aodev);
> > +    return;
> > +}


> > +    /* No other code will traverse device list, update json with removal info */
> > +    if (aodev->update_json) {
> When is update_json set to true?
> 
> Note that in DEFINE_DEVICE_ADD update_json is set to true. But in you
> patch there doesn't seem to be code that does this.

Right, somehow this got lost.

> > +        /* Replace the item in the domain config */
> > +        DEVICE_ADD(vscsi, vscsis, aodev->dev->domid, vscsi, COMPARE_VSCSI, &d_config);
> Actually, what fields inside vscsi structure are updated in this case?

The essential part is the ->vscsi_devs array, and the ->remove flag in
each subdevice.

> > +/* Extended variant of DEFINE_DEVICE_REMOVE to handle reconfigure */
> 
> I have a very dumb question, what is "reconfigure"? Is this property of
> vscsi device? What does this suppose to do?

Its XenbusStateReconfiguring, which tells the backend to rescan the
vscsi-devs directory. vscsi and pci do this because they have the
single_host-many_devices concept.

Thanks for the review. I will resend the current state later today.

Olaf