From patchwork Thu Jun 21 17:53:44 2018 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Bart Van Assche X-Patchwork-Id: 10480313 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4451060230 for ; Thu, 21 Jun 2018 17:53:52 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 328C2287E8 for ; Thu, 21 Jun 2018 17:53:52 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2742329112; Thu, 21 Jun 2018 17:53:52 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, MAILING_LIST_MULTI,RCVD_IN_DNSWL_HI,T_DKIM_INVALID,T_TVD_MIME_EPI autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id CE784287E8 for ; Thu, 21 Jun 2018 17:53:50 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932519AbeFURxu (ORCPT ); Thu, 21 Jun 2018 13:53:50 -0400 Received: from esa2.hgst.iphmx.com ([68.232.143.124]:9771 "EHLO esa2.hgst.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753725AbeFURxs (ORCPT ); Thu, 21 Jun 2018 13:53:48 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1529604504; x=1561140504; h=from:to:cc:subject:date:message-id:references: in-reply-to:mime-version; bh=Ut4peVg7cPD3ZwwGcrmSJwajcLqr/3YqVRNKnO0Dljw=; b=bCHbmsnuSqGoZGzWL1nThyNti3RtcPqUt4DfNFJ1XInsWrf/vfc4mMJD KYmOCnavzZ8+PWu84sTBq7qqyJsIKw8SllGeMkMexlIIWGZIqIDu7Uwbk hEaM6EWW+DTDOXBFNcJNoe4VTHrrGARMUQzwUbcA00VepcBSOExKFJMZI WGIjfRztYz2PKqTnYVhv6T8DfY4HKUWY8j1VD/4CASW0r4zKLg1zapT5m L41XkiUwmfmBWXHc0s53o+vHq9ae25j7026QhLKtxmL1yKZdYs6XtTPeg OdioxvxyleQF1t7CHxnEku6DNF9f/8J8IdocYKxZ6w7ZKtbtsiczgC6rX w==; X-IronPort-AV: E=Sophos;i="5.51,253,1526313600"; d="scan'208,223";a="178131611" Received: from mail-by2nam01lp0182.outbound.protection.outlook.com (HELO NAM01-BY2-obe.outbound.protection.outlook.com) ([216.32.181.182]) by ob1.hgst.iphmx.com with ESMTP; 22 Jun 2018 02:08:23 +0800 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sharedspace.onmicrosoft.com; s=selector1-wdc-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=5ifRNeiyuSPAmYi3kO23wb+fI8+xLPK++xQ5+CzRyLE=; b=VnDffTnD+A+hKHwSeQXrnZxxluP9AweiCN8W8nMveYE8y2YLvyQcnUhwYyIQJ4LfGFG7pNcbTPzetX45GmPtxjVOxvnOnn/rjECjhJzgjU9yKG1MLgmgLhJm/cz2nAIKztY+thCnSjf/ez0KEEomcyNcSOOnvsljACDmJMk8COk= Received: from MWHPR04MB1198.namprd04.prod.outlook.com (10.173.48.151) by MWHPR04MB0399.namprd04.prod.outlook.com (10.173.48.16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.863.19; Thu, 21 Jun 2018 17:53:45 +0000 Received: from MWHPR04MB1198.namprd04.prod.outlook.com ([fe80::9de4:3c8d:39af:e4c5]) by MWHPR04MB1198.namprd04.prod.outlook.com ([fe80::9de4:3c8d:39af:e4c5%8]) with mapi id 15.20.0863.016; Thu, 21 Jun 2018 17:53:45 +0000 From: Bart Van Assche To: "mchristi@redhat.com" , "nab@linux-iscsi.org" CC: "hch@lst.de" , "hare@suse.com" , "target-devel@vger.kernel.org" Subject: Re: [PATCH 14/14] target: Fix handling of removed LUNs Thread-Topic: [PATCH 14/14] target: Fix handling of removed LUNs Thread-Index: AQHTsaxj40eZmZwmkUKpxupA87/gmaRqm/YAgAESR4A= Date: Thu, 21 Jun 2018 17:53:44 +0000 Message-ID: <8998eb5e118201e7922cc70296ddc7ed749ab5bb.camel@wdc.com> References: <20180301222632.31507-1-bart.vanassche@wdc.com> <20180301222632.31507-15-bart.vanassche@wdc.com> <5B2B0012.4060704@redhat.com> In-Reply-To: <5B2B0012.4060704@redhat.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: yes X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=Bart.VanAssche@wdc.com; x-originating-ip: [199.255.44.174] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; MWHPR04MB0399; 7:0U3Y/B524IDXoigC0zdPEAij84lV3wKmQBMaZ9tJoV1gebWJ6R07vFu2HjfQtkciTB1Z3NFNYdkA4mj730A84defSxUV+exomUZP3SVeLjM8Dp9Pn1QX3Kv9VCNUgXEQYnzI+DfCPGJBl1eIODIOk7001kyWVarrgCr594shr53szXXKljr9qOantfnlJA70QuNZr/s2j3tgKjh1EWu65h/L2f9dXq/i+Zesy9RsGQdi8QrNmLj90zopjsAXRf+y; 20:9+7q7BJfze1vRsR0qSSrmLk87cfCPtLKVgLRz8+oafIUzx4DzDOjX8yrnyknHGpZv9kgLM+xlytP5o4P/x+l2YT4Z8QheMw+wqwddQtjEQFk825MifEOftCWZuTxR1+TCb0A3ZcocyBd095wwnOV5q/QqgUDiCAkdUXcb4ZrHqQ= x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: 63e37834-f489-4c9b-3db3-08d5d79feee7 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: UriScan:; BCL:0; PCL:0; RULEID:(7020095)(4652020)(4534165)(4627221)(201703031133081)(201702281549075)(5600026)(711020)(48565401081)(2017052603328)(7153060)(49563074)(7193020); SRVR:MWHPR04MB0399; x-ms-traffictypediagnostic: MWHPR04MB0399: wdcipoutbound: EOP-TRUE x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(102415395)(6040522)(2401047)(5005006)(8121501046)(10201501046)(93006095)(93001095)(3231254)(944501410)(52105095)(3002001)(6055026)(149027)(150027)(6041310)(20161123564045)(20161123558120)(20161123562045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123560045)(6072148)(201708071742011)(7699016); SRVR:MWHPR04MB0399; BCL:0; PCL:0; RULEID:; SRVR:MWHPR04MB0399; x-forefront-prvs: 07106EF9B9 x-forefront-antispam-report: SFV:NSPM; SFS:(10019020)(396003)(39380400002)(39860400002)(376002)(346002)(366004)(199004)(189003)(51444003)(229853002)(6512007)(486006)(4326008)(2906002)(3846002)(6116002)(478600001)(6486002)(6246003)(53936002)(25786009)(110136005)(316002)(186003)(72206003)(54906003)(36756003)(99286004)(76176011)(14454004)(11346002)(476003)(2616005)(102836004)(446003)(6506007)(6436002)(26005)(59450400001)(5660300001)(7736002)(97736004)(305945005)(66066001)(3280700002)(2501003)(5250100002)(5890100001)(118296001)(3660700001)(99936001)(106356001)(68736007)(8676002)(86362001)(2900100001)(81166006)(8936002)(81156014)(105586002); DIR:OUT; SFP:1102; SCL:1; SRVR:MWHPR04MB0399; H:MWHPR04MB1198.namprd04.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; x-microsoft-antispam-message-info: ytY1BxIpPdqZo9AqnnD9SvHH+C0UOhy3tipuIXLNIvqeLv0p2980Pg3mphEkBsAwfJBquTvAguNFjHm4g3KKzfbLV1G1346imDlCSWXXvDfG4Cy0dBYq6cZCN2aHCqM5c2h+P4t+im0wUsmkBNvgWfp8Fz9BvMvOpZYmyag9wsz95bzX/3NuxwfpWCNKe72heELgv8P0fUDXksLhrvfAv55pVV8U+Yf3YcqjjxGWQbWkpqtY4uJ3VhPzbC9thSMBb7ycVx6ovTjdWIes8vLUaMvmDNXX0VmjcJJN9yCJ6eHf2CUKkinFaGSynTDwC09EqBI/t9MoVyC6hN2kp1+0xg== spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM MIME-Version: 1.0 X-OriginatorOrg: wdc.com X-MS-Exchange-CrossTenant-Network-Message-Id: 63e37834-f489-4c9b-3db3-08d5d79feee7 X-MS-Exchange-CrossTenant-originalarrivaltime: 21 Jun 2018 17:53:44.8825 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86 X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR04MB0399 Sender: target-devel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: target-devel@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On Wed, 2018-06-20 at 20:32 -0500, Mike Christie wrote: > I was in the middle of fixing up the sense key value issue in the patch > like we discussed before (use illegal request instead of unit > attention), but it looks like there was another bug. If we have 2 > commands going to the same device and they run target_scsi3_ua_check and > see ua_count=1 TCM_CHECK_CONDITION_UNIT_ATTENTION is returned for both. > They then call core_scsi3_ua_for_check_condition and one of them > deallocates a UA dropping ua_count=0. The second command then runs the > !atomic_read(&deve->ua_count) check and sees the other command has > decremented it. Or the second one might have passed the ua_count check > in core_scsi3_ua_for_check_condition and it runs the loop but nothing is > found since the first command has already removed it. We then return > bogus asc/ascq values. > > In the attached patch I just return busy status for this race case. It > seemed easier than trying to add more locking and error handling. > > Some notes/questions on some of the code the patch touched though. > > If translate_sense_reason failed due to a short buffer it returned > -EINVAL. transport_send_check_condition_and_sense would then end up > calling transport_handle_queue_full/transport_complete_qf and that would > just end up failing the command with > TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE. Do you know if that long journey > intended or just an accident? > > In this patch I fixed that so it just translated the error to > TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE immediately. If the patch is ok > and we meant to return that error code for the short buffer then I can > break that out into another patch. Hello Mike, That's an interesting observation. However, I think that the race described above can be fixed with fewer code changes. Can you have a look at the three attached (untested) patches? Regarding translate_sense_reason() failing due to a short buffer: I don't think that the current behavior in case of a short sense buffer is correct. It's probably better to return a sense buffer that is incomplete and with correct KEY / ASC / ASCQ values instead of modifying these values. How about changing the code at the end of translate_sense_reason() as follows? if (si->add_sector_info) WARN_ON_ONCE(scsi_set_sense_information(buffer, cmd->scsi_sense_length, cmd->bad_sector) < 0); return 0; Thanks, Bart. From fac94854486b98dc04c89c21a4b2f24f895bbafe Mon Sep 17 00:00:00 2001 From: Bart Van Assche Date: Thu, 21 Jun 2018 10:19:05 -0700 Subject: [PATCH 3/3] target: Remove se_dev_entry.ua_count se_dev_entry.ua_count is only used to check whether or not se_dev_entry.ua_list is empty. Use list_empty_careful() instead. Checking whether or not ua_list is empty without holding the lock that protects that list is fine because the code that dequeues from that list will check again whether or not that list is empty. Signed-off-by: Bart Van Assche Cc: Mike Christie Cc: Christoph Hellwig Cc: Hannes Reinecke --- drivers/target/target_core_device.c | 1 - drivers/target/target_core_ua.c | 12 ++---------- include/target/target_core_base.h | 1 - 3 files changed, 2 insertions(+), 12 deletions(-) diff --git a/drivers/target/target_core_device.c b/drivers/target/target_core_device.c index e8d2a7f22382..d91cbff57680 100644 --- a/drivers/target/target_core_device.c +++ b/drivers/target/target_core_device.c @@ -336,7 +336,6 @@ int core_enable_device_list_for_node( return -ENOMEM; } - atomic_set(&new->ua_count, 0); spin_lock_init(&new->ua_lock); INIT_LIST_HEAD(&new->ua_list); INIT_LIST_HEAD(&new->lun_link); diff --git a/drivers/target/target_core_ua.c b/drivers/target/target_core_ua.c index d2fe3d98c335..49ccac620fcf 100644 --- a/drivers/target/target_core_ua.c +++ b/drivers/target/target_core_ua.c @@ -55,7 +55,7 @@ target_scsi3_ua_check(struct se_cmd *cmd) rcu_read_unlock(); return 0; } - if (!atomic_read(&deve->ua_count)) { + if (list_empty_careful(&deve->ua_list)) { rcu_read_unlock(); return 0; } @@ -154,7 +154,6 @@ int core_scsi3_ua_allocate( &deve->ua_list); spin_unlock(&deve->ua_lock); - atomic_inc_mb(&deve->ua_count); return 0; } list_add_tail(&ua->ua_nacl_list, &deve->ua_list); @@ -164,7 +163,6 @@ int core_scsi3_ua_allocate( " 0x%02x, ASCQ: 0x%02x\n", deve->mapped_lun, asc, ascq); - atomic_inc_mb(&deve->ua_count); return 0; } @@ -196,8 +194,6 @@ void core_scsi3_ua_release_all( list_for_each_entry_safe(ua, ua_p, &deve->ua_list, ua_nacl_list) { list_del(&ua->ua_nacl_list); kmem_cache_free(se_ua_cache, ua); - - atomic_dec_mb(&deve->ua_count); } spin_unlock(&deve->ua_lock); } @@ -263,8 +259,6 @@ bool core_scsi3_ua_for_check_condition(struct se_cmd *cmd, u8 *key, u8 *asc, } list_del(&ua->ua_nacl_list); kmem_cache_free(se_ua_cache, ua); - - atomic_dec_mb(&deve->ua_count); } spin_unlock(&deve->ua_lock); rcu_read_unlock(); @@ -304,7 +298,7 @@ int core_scsi3_ua_clear_for_request_sense( rcu_read_unlock(); return -EINVAL; } - if (!atomic_read(&deve->ua_count)) { + if (list_empty_careful(&deve->ua_list)) { rcu_read_unlock(); return -EPERM; } @@ -327,8 +321,6 @@ int core_scsi3_ua_clear_for_request_sense( } list_del(&ua->ua_nacl_list); kmem_cache_free(se_ua_cache, ua); - - atomic_dec_mb(&deve->ua_count); } spin_unlock(&deve->ua_lock); rcu_read_unlock(); diff --git a/include/target/target_core_base.h b/include/target/target_core_base.h index ca59e065c1fd..7a4ee7852ca4 100644 --- a/include/target/target_core_base.h +++ b/include/target/target_core_base.h @@ -638,7 +638,6 @@ struct se_dev_entry { atomic_long_t total_cmds; atomic_long_t read_bytes; atomic_long_t write_bytes; - atomic_t ua_count; /* Used for PR SPEC_I_PT=1 and REGISTER_AND_MOVE */ struct kref pr_kref; struct completion pr_comp; -- 2.17.1