Messages in this thread |  | | Date | Tue, 24 Feb 2015 10:12:53 -0500 | From | Boris Ostrovsky <> | Subject | Re: [PATCH V2 2/3] xen: scsiback: add LUN of restored domain |
| |
On 02/24/2015 01:06 AM, Juergen Gross wrote: > On 02/18/2015 06:02 PM, Boris Ostrovsky wrote: >> On 02/17/2015 02:02 AM, Juergen Gross wrote: >>> When a xen domain is being restored the LUN state of a pvscsi device >>> is "Connected" and not "Initialising" as in case of attaching a new >>> pvscsi LUN. >>> >>> This must be taken into account when adding a new pvscsi device for >>> a domain as otherwise the pvscsi LUN won't be connected to the >>> SCSI target associated with it. >> >> >> scsiback_do_add_lun() is being called from scsiback_frontend_changed() >> which eventually sets the state to Connected. Is the added test of >> 'try' an optimization or is it needed for correctness? > > It's needed for correctness. In case of resume the translation entry > will be already existing, thus scsiback_add_translation_entry() will > return a value unequal to zero. We don't want to set the device state > to "Closed" in this case.
Right, I understand that. I thought that after you
xenbus_printf(XBT_NIL, info->dev->nodename, state, "%d", XenbusStateClosed);
in scsiback_do_add_lun() you'd eventually
xenbus_switch_state(dev, XenbusStateConnected);
in scsiback_frontend_changed().
What I didn't realize was that this switch would only happen if dev->state is not XenbusStateConnected, which it still will be, even after xenbus_printf(XenbusStateClosed).
So
Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
> > Juergen > >> >> -boris >> >> >>> >>> Signed-off-by: Juergen Gross <jgross@suse.com> >>> --- >>> drivers/xen/xen-scsiback.c | 19 ++++++++++++++----- >>> 1 file changed, 14 insertions(+), 5 deletions(-) >>> >>> diff --git a/drivers/xen/xen-scsiback.c b/drivers/xen/xen-scsiback.c >>> index 9faca6a..9d60176 100644 >>> --- a/drivers/xen/xen-scsiback.c >>> +++ b/drivers/xen/xen-scsiback.c >>> @@ -992,7 +992,7 @@ found: >>> } >>> >>> static void scsiback_do_add_lun(struct vscsibk_info *info, const >>> char *state, >>> - char *phy, struct ids_tuple *vir) >>> + char *phy, struct ids_tuple *vir, int try) >>> { >>> if (!scsiback_add_translation_entry(info, phy, vir)) { >>> if (xenbus_printf(XBT_NIL, info->dev->nodename, state, >>> @@ -1000,7 +1000,7 @@ static void scsiback_do_add_lun(struct >>> vscsibk_info *info, const char *state, >>> pr_err("xen-pvscsi: xenbus_printf error %s\n", state); >>> scsiback_del_translation_entry(info, vir); >>> } >>> - } else { >>> + } else if (!try) { >>> xenbus_printf(XBT_NIL, info->dev->nodename, state, >>> "%d", XenbusStateClosed); >>> } >>> @@ -1060,10 +1060,19 @@ static void scsiback_do_1lun_hotplug(struct >>> vscsibk_info *info, int op, >>> >>> switch (op) { >>> case VSCSIBACK_OP_ADD_OR_DEL_LUN: >>> - if (device_state == XenbusStateInitialising) >>> - scsiback_do_add_lun(info, state, phy, &vir); >>> - if (device_state == XenbusStateClosing) >>> + switch (device_state) { >>> + case XenbusStateInitialising: >>> + scsiback_do_add_lun(info, state, phy, &vir, 0); >>> + break; >>> + case XenbusStateConnected: >>> + scsiback_do_add_lun(info, state, phy, &vir, 1); >>> + break; >>> + case XenbusStateClosing: >>> scsiback_do_del_lun(info, state, &vir); >>> + break; >>> + default: >>> + break; >>> + } >>> break; >>> >>> case VSCSIBACK_OP_UPDATEDEV_STATE: >>> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe >> linux-kernel" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> Please read the FAQ at http://www.tux.org/lkml/ >> >
|  |