Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
To:     Sagi Grimberg <sagi@grimberg.me>
CC:     Daniel Wagner <dwagner@suse.de>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        Chaitanya Kulkarni <kch@nvidia.com>,
        Shin'ichiro Kawasaki <shinichiro@fastmail.com>
Subject: Re: [PATCH blktests v2 2/2] nvme-rc: Cleanup fc resource before
 module unloading
Thread-Topic: [PATCH blktests v2 2/2] nvme-rc: Cleanup fc resource before
 module unloading
Thread-Index: AQHZe09FVEwtRfzey0GWUGL193fJOg==
Date:   Sun, 30 Apr 2023 10:34:00 +0000
Message-ID: <gqeyqpv2idkkkf7jajdxbgj4fi5tnlbt7tmuudwxq3gul3w56y@ijwb4axr52dl>
References: <20230419084757.24846-1-dwagner@suse.de>
 <20230419084757.24846-3-dwagner@suse.de>
 <3ec250a9-54c7-3e0d-2463-f8faf15cdb58@grimberg.me>
 <3euieyvxpsww5p4m457pzupfipbnbv3atxauh2lrp54gqmqcco@hs7ytoop5osf>
 <1b060988-fc63-4a86-33b8-88ba61c9358f@grimberg.me>
In-Reply-To: <1b060988-fc63-4a86-33b8-88ba61c9358f@grimberg.me>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?vTlmm/QCIRiDzEqeMUdtUdKe5vk5c6dL2wDv6diyFmmsocNxw9VqbqFwWS10?=
 =?us-ascii?Q?NhCm6faAWVXMHGSkUkJWPEmyGvU1Q8b56AJvsKiKVB0yrp7HLB+tlOi0wJG2?=
 =?us-ascii?Q?6l7Ba4tuQ1D8EAifd4O9oMX+Va8VTLbDhXJUVzLF7so483frI7HuM2N+TO1e?=
 =?us-ascii?Q?j1NQ+ZlzjWfUVAvAGb9TPRnaNz4bCInxxAP10UI87nUTeJq5u63KI4HdqRel?=
 =?us-ascii?Q?jtyQT6p+rYVd7CU1nvjhDLGrr2KeM20VIBPrBGik5NFGNExynnHs5j1/vxPX?=
 =?us-ascii?Q?1KgbcRoOyX36VOES8KKZc+GGOwWqUfUZ0xCBRHy8PH2T41cnx09g/wMYEaLI?=
 =?us-ascii?Q?ua5XkuByChmFq6+wUJdGJxpjs3qLtvwCOtuQKFYHS1hxyb2bbKUHTWt1Su1g?=
 =?us-ascii?Q?wTwfjdsJFtXqTiR6GQqTdMJvei+pv3ZnlyuNp1uBpETIQrJ7mtwJbBZomyDE?=
 =?us-ascii?Q?iF40rkbZ3wxEPtdgyVPU4cCHf/6JmnF0qN6a6AUU5kZA16Zdwaz+Wdn33RS4?=
 =?us-ascii?Q?af9DJ0GjV2LnrlXzDfM0gEhlZORdXqLneocmCuU8FFQT4V5RLuQkT1ApfwwC?=
 =?us-ascii?Q?lEk/qx8EjHkgozTtJC9OTUHG1JrDm4LL3CdBmCle5oHNb8I9EyTFX8VVdGWG?=
 =?us-ascii?Q?mpGrAnza3TzNoJJluCSMPZlLYFwLsun56uAKnBOi6GbgaomdBMkFqb7mM072?=
 =?us-ascii?Q?bPO8q8wkQykpI3eCGP6T633kMaUphzbVbVHd0Q577Fo4fFGrYIQ7ZiUOKj3y?=
 =?us-ascii?Q?0g5vGwGr/ADnbWbTQyAcMuO/b4h0d55DMN8pU0PSKfLgiZaY1oLRTAseg77j?=
 =?us-ascii?Q?ZDCWgLe2DI/uAheYCObT0UV67eisV2lsuwvMO3bveK7FU5bGjn49GWy8I0Fa?=
 =?us-ascii?Q?ovE5GCpiTxskLH/I0FWUOshstEHrQ+Ho4B2VM7KP3pC+UnNaSNQtFEhirc+b?=
 =?us-ascii?Q?OqWchlMuxXxOzXz7kUMg3tdBstZX+5K+bRqtteXe4XitRrIB1UlWJcg/+7P0?=
 =?us-ascii?Q?/Kh/YI/gML3Q0Ytx61tr0zcVGxLs+pl1RRCmKI0XsFKPaN8sdeSHrPTPBdck?=
 =?us-ascii?Q?ofJvisZa7IClkbJbpMd1kSrlgFwoE5H2XKNQVn4V2oS1i6ok/TU+wztK8Fl9?=
 =?us-ascii?Q?IiTWiB3QV8jbhesYBm2S8hrsiq8n0Xv3rNycGwuAdqVrQAip17LaxOAzUZ/t?=
 =?us-ascii?Q?08uy39g4S5dKN/dZW7iHdMMfE0ZDGpo0tCmCPZDNAYWQ9HzF/kQ/jYCV5RyZ?=
 =?us-ascii?Q?BZ+84VpoEylQj5ZVnBscfu2xbcRwuexGYbpb7FPwlW4wqIss/fTTf1Zobu68?=
 =?us-ascii?Q?T/HqAEAowsEXdR++oVedTNVW3esWk66Hld1o7XBUhajuJpe3JDKTJkMJgKaR?=
 =?us-ascii?Q?6/2s7F8i/y5AzsCIZW960DlSoaspsWJ9eD5hhT16iAnQs02wyCOivBcGRRS+?=
 =?us-ascii?Q?EjvCS3nwwDdA9Q/Sp2JX+vHmh2ih8IgMPj3H3sw7aEpaUoX/TcemgSXw5H5E?=
 =?us-ascii?Q?RBRjQ16B9C+6GTXnAWjbuqygft2t33nA00HkjMWyyOLWii8jHu58mCm2fgHW?=
 =?us-ascii?Q?MuAUCqzx8VNJqgNWVCp8eRNNcuJp5if4zlP+2+5SivS6/6Kg1rfCTm7SQqgQ?=
 =?us-ascii?Q?1cKVzh5ipU0+CMEprNXZ/6Y=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <99DFAA1526CAA141BE76DF692111635C@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: =?us-ascii?Q?UVSYdD7DFUg7xaHxfq82t6T+m3DcLi+gIq7RO/MTZKMmC7CPzNepzraJd8Gp?=
 =?us-ascii?Q?e2fLR6Yqw45YRwt+eTIJ73DniPCOB1Sm3si2YHI89MuadK9S5cuA8zDKcd6d?=
 =?us-ascii?Q?YDhy/AilUJnjrk5+WcRRrzD2NP7NsTNZug+QXb7S3rmlZWreagZrLCfU9+s9?=
 =?us-ascii?Q?VFO1TQk/WOElr+7eNlb72Xfui5bQCsaMcdgDT0ouKLaKlbDNVPq6pw1MaK+l?=
 =?us-ascii?Q?PJmVgK1NiTSHYjtIfPPuP46o7myO8qQae7kqjgKDUrtgQIdz9BHdGalZNT+D?=
 =?us-ascii?Q?vVQ38W1fh0AJRitGZrFgy0W0Ao4jyqdPsD6Q/ADOMVUvfALBNfW8TnfHoJVC?=
 =?us-ascii?Q?Tsgi5tvBiFloNA0SeOz8OqeoG37dAKTPEkFjxkyTrxEOjwGPx5BXNQVNvAr/?=
 =?us-ascii?Q?IeJlo/zB+NSaKBGYkQ/oDJkHbZb45LP0Cnof8s2cATSlSq8I69+lo/3Ru4Qf?=
 =?us-ascii?Q?r91p7iHorFAcSJx/urI5zOi8r9wgSrB663FPDs+ufaXefeF4jS6k/2TrH+6Z?=
 =?us-ascii?Q?z6oFtFSzBhtoqVfhNVKJIvjdfJrTak7JDlkV3XUF2/19iiAhl0OCZOa+dHW/?=
 =?us-ascii?Q?py41vSBvbiwXJIbXjWRuLNehii418X/39WrWA3Lh+5J+8A1A3C41IJ08imJI?=
 =?us-ascii?Q?p8Y1mLa/F+6hbl14U7ieofeCHGPy1yZyhPz1LW7LuYFFaC3L8tNADyomEkKD?=
 =?us-ascii?Q?8raOeccvY+a9oZBBhlC7lgpmw9QKaevxK2rAW/mIVFIKRCFwMz/B0M5iIDfg?=
 =?us-ascii?Q?j0DvkPmhVq0VRJUBP1ikFU2frNpKCvAHJPfDdAne/pZuwIEIT/wH2cgO6d8w?=
 =?us-ascii?Q?bynaRZoiBIytyIS5YJ+YJpdvWtRl+Sb6fVrOoBmHZpyDDOSZpHI8iKYyuxDf?=
 =?us-ascii?Q?6kcl/wWpb/4peLp+YbB1yd32g0ZQB1h+Y0jY9pMMN9Thg9qFTgE2DPPUiM0N?=
 =?us-ascii?Q?9kM3lg9xirVN0gq23z3mUw=3D=3D?=
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM8PR04MB8037.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e7da095d-19b8-472f-216c-08db4966690c
X-MS-Exchange-CrossTenant-originalarrivaltime: 30 Apr 2023 10:34:00.0840
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: W+sGrws7USIyCEW0wpeK63gkTny8bTNsGO8u8w6jhE4lCWlPpytZOimxuABLI6m9PDDOODh1eYsKdPtaGR41VcEbtuFEByrAy6AuqCYaE5k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR04MB8145
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,SPF_HELO_PASS,
        SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham 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 Apr 19, 2023 / 13:45, Sagi Grimberg wrote:
>=20
> > > > Before we unload the module we should cleanup the fc resources firs=
t,
> > > > basically reorder the shutdown sequence to be in reverse order of t=
he
> > > > setup path.
> > >=20
> > > If this triggers a bug, then I think it is a good idea to have a
> > > dedicated test that reproduces it if we are changing the default
> > > behavior.
> >=20
> > Right, though I would like to tackle one problem after the other, first=
 get fc
> > working with the 'correct' order.
> >=20
> > > > While at it also update the rdma stop_soft_rdma before the module
> > > > unloading for the same reasoning.
> > >=20
> > > Why? it creates the wrong reverse ordering.
> > >=20
> > > 1. setup soft-rdma
> > > 2. setup nvme-rdma
> > >=20
> > > 2. teardown nvme-rdma
> > > 1. teardown soft-rdma
> > >=20
> > > I don't think we need this change. I mean it is a good test
> > > to have that the rdma device goes away underneath nvme-rdma
> > > but it is good for a dedicated test.

I agree that the new test case is good.

> >=20
> > I was woried about this setup sequence here:
> >=20
> > 	modprobe -q nvme-"${nvme_trtype}"
> > 	if [[ "${nvme_trtype}" =3D=3D "rdma" ]]; then
> > 		start_soft_rdma
> >=20
> > The module is loaded before start_soft_rdma is started, thus I thought =
we should
> > do the reverse, first call stop_soft_rdma and the unload the module.
>=20
> They should be unrelated. the safe route is to first remove the uld and
> then the device.

Sagi, this comment above was not clear for me. Is Daniel's patch ok for you=
?

IMO, it is reasonable to "do clean-up in reverse order as setup" as a gener=
al
guide. It will reduce the chance to see module related failures when the te=
st
cases do not expect such failures. Instead, we can have dedicated test case=
s for
the module load/unload order related failures. start_soft_rdma and
stop_soft_rdma do module load and unload. So I think the guide is good for =
those
helper functions also.