From mboxrd@z Thu Jan 1 00:00:00 1970 From: Olaf Hering Subject: Re: [PATCH v4 3/5] libxl: add support for vscsi Date: Wed, 6 May 2015 10:06:10 +0200 Message-ID: <20150506080610.GB28758@aepfle.de> References: <1429259460-30491-1-git-send-email-olaf@aepfle.de> <1429259460-30491-4-git-send-email-olaf@aepfle.de> <20150505135537.GE1455@zion.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <20150505135537.GE1455@zion.uk.xensource.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Wei Liu Cc: Stefano Stabellini , Ian Jackson , Ian Campbell , xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org 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