Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <137ec5f5-dbdc-99f0-e9b7-deeef5001a01@oracle.com>
Date:   Thu, 14 Apr 2022 10:22:22 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.0
Subject: Re: [PATCH 1/2] scsi: iscsi: introduce session UNBOUND state to avoid
 multiple unbind event
Content-Language: en-US
To:     Wenchao Hao <haowenchao@huawei.com>, Lee Duncan <lduncan@suse.com>,
        Chris Leech <cleech@redhat.com>,
        "James E . J . Bottomley" <jejb@linux.ibm.com>,
        "Martin K . Petersen" <martin.petersen@oracle.com>,
        open-iscsi@googlegroups.com, linux-scsi@vger.kernel.org,
        linux-kernel@vger.kernel.org
Cc:     linfeilong@huawei.com
References: <20220414014947.4168447-1-haowenchao@huawei.com>
 <20220414014947.4168447-2-haowenchao@huawei.com>
From:   Mike Christie <michael.christie@oracle.com>
In-Reply-To: <20220414014947.4168447-2-haowenchao@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Y3BPUDUvY2Z5YkRDVVhzaVVwTGcyMnlkN2w1OTdLVDE0NWt4YTk3Vk5vU21j?=
 =?utf-8?B?c0lCUWZrN3djWlRGWmVPdXJoa05jckZ4bnJhZnBmSEJMcmp0TmRkb2g4YzJB?=
 =?utf-8?B?MVNWY0F6WEtRWGNwR1VoTXJuMmhxT0U0MmpMZjFXb3RPUXZ2SnZuN1hva25T?=
 =?utf-8?B?b0xQSDYvbzV2bWlBUTJMczI2K0ZvNnJzbElHaDlPMnc5bm95N2NydWVKRVU1?=
 =?utf-8?B?d1lIRWp5eHRpYVZQdWI3eWpxOWhZU0NZWnk0MVI3c3p5WGpkVVI3Y0tTdXJE?=
 =?utf-8?B?SFoycGFnZ3NPS2tQOVp5b0hjSnQ5bXNjUTJobVU1NVQydFFyVmUwRjFHc0g3?=
 =?utf-8?B?KzFFRVZLaVFGZGtXM3pMTkc5VVVuZTQ4eTVtRVZGTUlXTGtlRHRnc2RYbmEy?=
 =?utf-8?B?a1B3eVJnSENtaXMrcjdtd24wcEZKMFZiRFBrU1I1ODhPQndRR2RNSGE4YVVI?=
 =?utf-8?B?TGJSd2wwb1F0OFg2dU9MZ1pMOFU5TmwrR25GeU9GRVQ1WTBGRk8vQ0pmdE9v?=
 =?utf-8?B?MnpBazNqUEhuWVE2N3M3QTdDd2tmUVJLdGlCZ0Q2aHdKdVdqT0p3MHVvMjFl?=
 =?utf-8?B?UXFWVnVTV1A5TW1ER0FnaVFtNWYxd285QWxCNDlsK3dYNnpPMEpyekNDK09Q?=
 =?utf-8?B?TXQ3TjZaWXBFRWk4SEZlQVZJNm83TWp6SzBhaExKNG9OUUY5RzM0NnlsZ2N3?=
 =?utf-8?B?TGRMb0E2ZlJpV2JlWkUwR00zSUxHMEpPN3BMbW5FeDBEM1kyT0c4aE9USFJK?=
 =?utf-8?B?VWkyYVk5WExORFpoRU5jcU52SmxNbkJxdFJBaWFXaktIMWNNM3k4b2EyOU52?=
 =?utf-8?B?RE1XeVdNS0xIUUZoSkhESzRHRkozTWI1NUJWWjlvZ2ZvRWhhNmZtTUE1Mk84?=
 =?utf-8?B?c1ZZSXVvZURpcWhENE91Y0Q2TkVTaG93TDNwQys3Rm93M1lIK0ZBQTdMQmJj?=
 =?utf-8?B?Rk1sV1JNWjN1UFd1Z2xMd05uUk1YcHZ1emg3aE0vNlN4VHdBVE9qTXN4cUVH?=
 =?utf-8?B?M1FSSzNYRDZubDJibVJmelJBcmpsQjJ2eW9NcHlLOXFrUVZGQmFNMW5DZ2Nq?=
 =?utf-8?B?U1U2YXlPaWZkWmpUTVRUTGpSY0U2eTZIOGEzKy9RWHkreU9YY3dQNGVjQ29t?=
 =?utf-8?B?NzN0UnNPcDhjZ3BMeDRPeGl4VnB2WTQ2dzM3aTlSY3EwUmYwZlpGMFphcmt0?=
 =?utf-8?B?aFFwV29pM3NYd0ZaUlJGWVNhS21RMjNDNU1ZdDhIOGFGY0ZkQy94QTc0OEFR?=
 =?utf-8?B?VnQwd1hGNnB2aWVqV2tuZm0wZFJTUmlxajlKWDRsMUhzWFlsMWVZVzZJRkhi?=
 =?utf-8?B?RW8zcll5MXM2T0ZHeWwyUWNyVUJoaXN6OGhLV0h2alVzaWhuZTZYWHdST1VG?=
 =?utf-8?B?NkpnM294MEx2MTRseGNpZitUT09WbXE0SXE4TC94S1o3K2o5UU9QdElZdmR1?=
 =?utf-8?B?N2lwMVcrVnRMUDB1N0VOMk1WOHdFTnk2MWZ5bHJKVmV4Q3F0QlZHbjZOOHV4?=
 =?utf-8?B?b1VXNGJrZ25CTWl5SmpvYmszenJLdzh5eEFxSFd0Q1NmRisybDNCdmwrRmZE?=
 =?utf-8?B?OGE1WjFVU3ZraTROclpTV05kTlkvQjhKRWU3aDRILy9zWERzVzlWOWc0MXNK?=
 =?utf-8?B?VHJ5dUdtalhVamxEZEJaUmM5akVxSGpjUDl3ZjYzUWhiRTZRelk1eFRNR0tk?=
 =?utf-8?B?eUxxSUxTaWNnZUVvRUVKWWJRQWE0SUNYS1BSVk8wbzRnbzNUTk1DOHRuRDRM?=
 =?utf-8?B?azEvY3lxTXMwajVMZ3FubWFZU3lQZVZXTTZFVVMvTm1IV2hhM2YyU0ptbDR1?=
 =?utf-8?B?OE9mK1Z4RGNUL05xaUJPcEJiS0ZEaFFEaTlOaXlxWVc5RTdSd005aTZvMXdW?=
 =?utf-8?B?NmJxbkhrZVVhZU5abVpXdkRrZEpmTlk5M1Z6bkFJRHJ3ZmZIWm9GV3RDcGtO?=
 =?utf-8?B?UHVVNXFEYnZDbFYvdlBmSmRlUm02dkFhYVhWajF4Vmd1czJmWGh6K1JqMEtj?=
 =?utf-8?B?eWFuVWtUd0dNOTBnUGtTYUZ1UEdEYzNvTnFlSFJhUHo0bUsyaFZwSkFVRFo3?=
 =?utf-8?B?dVRNZHgwSGk4V202eG00eG9QTTA4YkRNRndtV29ITlZiRTJLWjNlb0R4ZG9H?=
 =?utf-8?B?T29FTjhFdGY0OTZIekUxdUVXOWNrZXpIZko5MU1hYTF1SFF5WUd3TFZrM2tG?=
 =?utf-8?B?b3NWRDV5NXhSZEtqY1h0Q3dEeUcraVc2ZHZKbis3V24ycElNZmUvSmw0Ris1?=
 =?utf-8?B?dXZSaElzbnJUNHloY1FUWkkrZlJFZGpqSGtzL2RjemlLQk9vWStXWVQ1MkJI?=
 =?utf-8?B?bUVqbi9GdG1BTTM1K1J6emRjblR0YkNhMWQ1ck4wSTAwa1FkZ0NQQXUzNzJT?=
 =?utf-8?Q?BeUCtMGf54GpYxck=3D?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 5311dfce-87c0-45d1-bd57-08da1e2a9382
X-MS-Exchange-CrossTenant-AuthSource: DM5PR10MB1466.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Apr 2022 15:22:24.0812
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: nzXZV/v+wXigGz/eB/MY8jyHeIxBZN3+/iqz5AzuVtiUwjGqD4KOrmDREu2AiGd8euQywkMMu8uxMd16UDLolA2a5yorPy3vlnO9pujGaBE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR10MB5774
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.486,18.0.858
 definitions=2022-04-14_04:2022-04-14,2022-04-14 signatures=0
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 adultscore=0
 phishscore=0 mlxscore=0 suspectscore=0 malwarescore=0 mlxlogscore=999
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2202240000
 definitions=main-2204140082
X-Proofpoint-GUID: PP47LRSUrYeNRRi5XAyCub-hcq_H2wS5
X-Proofpoint-ORIG-GUID: PP47LRSUrYeNRRi5XAyCub-hcq_H2wS5
X-Spam-Status: No, score=-5.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,NICE_REPLY_A,RDNS_NONE,SPF_HELO_NONE,
	T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
	lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 4/13/22 8:49 PM, Wenchao Hao wrote:
> Fix the issue of kernel send multiple ISCSI_KEVENT_UNBIND_SESSION event.
> If session is in UNBOUND state, do not perform unbind operations anymore,
> else unbind session and set session to UNBOUND state.
> 

I don't think we want this to be a state because you can have a session
with no target or it could be partially deleted and it could be in the
logged in or failed state. If scsi-ml is sending SYNC_CACHEs as part of
the target/device removal operation, and we lose the session then we could
go through recovery and the state will go from failed to logged in, and
your new unbound state will have been overwritten.

I think it might be better to have a new sysfs file, target_state, for
this where you could have values like scanning, bound, unbinding, and
unbound, or just a sysfs file, target_bound, that is bool.

> Reference:https://github.com/open-iscsi/open-iscsi/issues/338
> 

You should add a description of the problem in the commit, because that
link might be gone one day.


> Signed-off-by: Wenchao Hao <haowenchao@huawei.com>
> ---
>  drivers/scsi/scsi_transport_iscsi.c | 19 +++++++++++++++++--
>  include/scsi/scsi_transport_iscsi.h |  1 +
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_transport_iscsi.c b/drivers/scsi/scsi_transport_iscsi.c
> index 27951ea05dd4..97a9fee02efa 100644
> --- a/drivers/scsi/scsi_transport_iscsi.c
> +++ b/drivers/scsi/scsi_transport_iscsi.c
> @@ -1656,6 +1656,7 @@ static struct {
>  	{ ISCSI_SESSION_LOGGED_IN,	"LOGGED_IN" },
>  	{ ISCSI_SESSION_FAILED,		"FAILED" },
>  	{ ISCSI_SESSION_FREE,		"FREE" },
> +	{ ISCSI_SESSION_UNBOUND,	"UNBOUND" },
>  };
>  
>  static const char *iscsi_session_state_name(int state)
> @@ -1686,6 +1687,9 @@ int iscsi_session_chkready(struct iscsi_cls_session *session)
>  	case ISCSI_SESSION_FREE:
>  		err = DID_TRANSPORT_FAILFAST << 16;
>  		break;
> +	case ISCSI_SESSION_UNBOUND:
> +		err = DID_NO_CONNECT << 16;
> +		break;
>  	default:
>  		err = DID_NO_CONNECT << 16;
>  		break;
> @@ -1838,7 +1842,8 @@ int iscsi_block_scsi_eh(struct scsi_cmnd *cmd)
>  
>  	spin_lock_irqsave(&session->lock, flags);
>  	while (session->state != ISCSI_SESSION_LOGGED_IN) {
> -		if (session->state == ISCSI_SESSION_FREE) {
> +		if ((session->state == ISCSI_SESSION_FREE) ||
> +		    (session->state == ISCSI_SESSION_UNBOUND)) {
>  			ret = FAST_IO_FAIL;
>  			break;
>  		}
> @@ -1869,6 +1874,7 @@ static void session_recovery_timedout(struct work_struct *work)
>  		break;
>  	case ISCSI_SESSION_LOGGED_IN:
>  	case ISCSI_SESSION_FREE:
> +	case ISCSI_SESSION_UNBOUND:
>  		/* we raced with the unblock's flush */
>  		spin_unlock_irqrestore(&session->lock, flags);
>  		return;
> @@ -1957,6 +1963,14 @@ static void __iscsi_unbind_session(struct work_struct *work)
>  	unsigned long flags;
>  	unsigned int target_id;
>  
> +	spin_lock_irqsave(&session->lock, flags);
> +	if (session->state == ISCSI_SESSION_UNBOUND) {
> +		spin_unlock_irqrestore(&session->lock, flags);
> +		return;
> +	}
> +	session->state = ISCSI_SESSION_UNBOUND;
> +	spin_unlock_irqrestore(&session->lock, flags);
> +
>  	ISCSI_DBG_TRANS_SESSION(session, "Unbinding session\n");
>  
>  	/* Prevent new scans and make sure scanning is not in progress */
> @@ -4329,7 +4343,8 @@ store_priv_session_##field(struct device *dev,				\
>  	struct iscsi_cls_session *session =				\
>  		iscsi_dev_to_session(dev->parent);			\
>  	if ((session->state == ISCSI_SESSION_FREE) ||			\
> -	    (session->state == ISCSI_SESSION_FAILED))			\
> +	    (session->state == ISCSI_SESSION_FAILED) ||			\
> +	    (session->state == ISCSI_SESSION_UNBOUND))			\
>  		return -EBUSY;						\
>  	if (strncmp(buf, "off", 3) == 0) {				\
>  		session->field = -1;					\
> diff --git a/include/scsi/scsi_transport_iscsi.h b/include/scsi/scsi_transport_iscsi.h
> index 38e4a67f5922..80149643cbcd 100644
> --- a/include/scsi/scsi_transport_iscsi.h
> +++ b/include/scsi/scsi_transport_iscsi.h
> @@ -232,6 +232,7 @@ enum {
>  	ISCSI_SESSION_LOGGED_IN,
>  	ISCSI_SESSION_FAILED,
>  	ISCSI_SESSION_FREE,
> +	ISCSI_SESSION_UNBOUND,
>  };
>  
>  #define ISCSI_MAX_TARGET -1