Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752450AbbBXPOS (ORCPT ); Tue, 24 Feb 2015 10:14:18 -0500 Received: from aserp1040.oracle.com ([141.146.126.69]:35577 "EHLO aserp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751059AbbBXPOR (ORCPT ); Tue, 24 Feb 2015 10:14:17 -0500 Message-ID: <54EC94F5.8060005@oracle.com> Date: Tue, 24 Feb 2015 10:12:53 -0500 From: Boris Ostrovsky User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0 MIME-Version: 1.0 To: Juergen Gross , linux-kernel@vger.kernel.org, xen-devel@lists.xensource.com, konrad.wilk@oracle.com, david.vrabel@citrix.com Subject: Re: [PATCH V2 2/3] xen: scsiback: add LUN of restored domain References: <1424156569-13642-1-git-send-email-jgross@suse.com> <1424156569-13642-3-git-send-email-jgross@suse.com> <54E4C5A8.9070109@oracle.com> <54EC14F9.5010409@suse.com> In-Reply-To: <54EC14F9.5010409@suse.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-Source-IP: aserv0022.oracle.com [141.146.126.234] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4089 Lines: 118 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 > > Juergen > >> >> -boris >> >> >>> >>> Signed-off-by: Juergen Gross >>> --- >>> 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/ >> > -- 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/