Received-SPF: pass (google.com: domain of linux-kernel+bounces-163363-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Mon, 29 Apr 2024 22:25:57 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: "Verma, Vishal L" <vishal.l.verma@intel.com>, "Williams, Dan J"
	<dan.j.williams@intel.com>, "Jiang, Dave" <dave.jiang@intel.com>, "Schofield,
 Alison" <alison.schofield@intel.com>, "akpm@linux-foundation.org"
	<akpm@linux-foundation.org>
CC: "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-cxl@vger.kernel.org"
	<linux-cxl@vger.kernel.org>, "nvdimm@lists.linux.dev"
	<nvdimm@lists.linux.dev>, "linux-kernel@vger.kernel.org"
	<linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/4] dax/bus.c: fix locking for unregister_dax_dev /
 unregister_dax_mapping paths
Message-ID: <663080e5be58_148729453@dwillia2-mobl3.amr.corp.intel.com.notmuch>
References: <20240416-vv-dax_abi_fixes-v2-0-d5f0c8ec162e@intel.com>
 <20240416-vv-dax_abi_fixes-v2-2-d5f0c8ec162e@intel.com>
 <6630487879c61_148729427@dwillia2-mobl3.amr.corp.intel.com.notmuch>
 <d4047d21f0b88c5baee9140adb56e86afca24b3a.camel@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <d4047d21f0b88c5baee9140adb56e86afca24b3a.camel@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?C77XKp9k0qdnbiXFOp+ZGdlrl3xpgrlFto3Rk6rRqzsZNbQJgP69fu7ZSM?=
 =?iso-8859-1?Q?oGobuqx5BahsKN+Uh6vrvjKhATGe/0ATsVkaG46ainIfI/QTXs0dvhuvNT?=
 =?iso-8859-1?Q?vhEQeGvsoeGdldasDOmhKBu+2j73NiZkof+UUjPTGGLaeza7w350+bbaGQ?=
 =?iso-8859-1?Q?4H1X6hPEyDqDapl+S6OZyWH6XFVjx/Y4KCKfF4kbzoOzCTlR6byeMKvpLe?=
 =?iso-8859-1?Q?115DN40Ml4RpgU+9CFRzOq3CWA2MEfPv2fubJhW2KY4rhdo5ij76Zt/k1o?=
 =?iso-8859-1?Q?prjOQ8qGzG+pJ1ZEKU3s1kdf9hicJD6tzufohPu5uDx1TjB7VpnEhD+ZPA?=
 =?iso-8859-1?Q?0ebvRtdnma7uVij7RvfsntVWb/2GZe9BSBrFZQNzO5Ui7kL/FiErzhu2p9?=
 =?iso-8859-1?Q?6cRRAWUTpGr6Gf4juVsWhPSupZBdDHXpL1XDTKQCNTUP19y58/qdnbruBW?=
 =?iso-8859-1?Q?D3svGXfrTFoWLKMb8OYbJwLfyIVmiSCKYRRy2fq38bQgk87fYC0bXF02xu?=
 =?iso-8859-1?Q?7Ej1EACFModta1x1rHnA561vL8NsMwbzhqXoYPBSgpanFi6uK2AWxfCbJg?=
 =?iso-8859-1?Q?ZxqjQhQtZ8mnXf0WVAinxlD1vluhh5p1MYKGQD9b9Aa6ZUu0vB0ENn8Qpx?=
 =?iso-8859-1?Q?HG+X4+oiwCXmxnl8FMcKCMiniDNEcOe9keQVhrfZCr1CJA/cHNI1mowv15?=
 =?iso-8859-1?Q?J6had9cZiwTIxdLbqfvqhmokLr0lHr9LxPIMRNHrJ7dRm2WZdfleFxNcGf?=
 =?iso-8859-1?Q?1/5Gsrc9eNNg5dDG7Gsvl2LCMYpoKBgSVDdjWoN2ezi/g+BU0e7DS64smF?=
 =?iso-8859-1?Q?1d8ZgGEvH5MQiB+X1btjbKWbXtSYd7nApDLfjtbtObsYEmcK5u/4g10xKq?=
 =?iso-8859-1?Q?m/JpmR5EwRek5eWUSPhOfQ3n4xhaBCxs/rJ1IY48yfNy+cgMMH6VJnnxeT?=
 =?iso-8859-1?Q?8FsTrNS94HkqezPr0pWrfSXhX3jjmTcHa+vb+8zue3aa9ClN6JU1k6KsI8?=
 =?iso-8859-1?Q?LLlxGPGj7hwyZ6L2iAqlvc+RUC1ScnwcwtiGqK1XZzCV/6MdDICWRblKhg?=
 =?iso-8859-1?Q?WKxMvUiOHeM5AaseoQMYPps6JWTfgACTWfYGPa8rbIvHohMQhbkIJh0j2x?=
 =?iso-8859-1?Q?dgYaHH2R8rBvdGRj1hqvSj83KbcSs5iXiu0z8shjtrZZzg60qwTZNrdbge?=
 =?iso-8859-1?Q?OQahDm/IjegbljI3OOHJfsQbaJuhfEtHBynwKhybajQUzTlWjLdM146a3d?=
 =?iso-8859-1?Q?3/jiDP+0mNCGANrW4Sney2H5AeJNhAWq2k33fTSxTSBFX+8Sdie9Vx0rec?=
 =?iso-8859-1?Q?uHfhkwLgnumpbLJb94fRhom3ObqnjjLpulS0LUbH8WA6HbDcbtx6r3shsf?=
 =?iso-8859-1?Q?ief0BZhAMymYxfKGPKNMi5EaJqZf7+5Xtqd3Qn2UA1l7YwZTxXuBcuyEHf?=
 =?iso-8859-1?Q?xCLsSA47VBDMSTpj+XZb2z71VitX8d8o81N1t0UbmFE0Jdh3jaEuY9Qlea?=
 =?iso-8859-1?Q?mu7HHaBXNsxKUsFYbZ3N6mZEuVWhViwXpLA4sH5s29asu+MbSL42ww8VcF?=
 =?iso-8859-1?Q?IXZt6XfyB7KNNLCJ62BAd4lrWOeLzgrVgjF3whLBxB1f5A/bHtwsPJhRXn?=
 =?iso-8859-1?Q?I+JRaGF5gD+zeYF0fEF57heT2kjsQ3V86J8RjkxQi0zW02SK+7eEVkzg?=
 =?iso-8859-1?Q?=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3e834071-af41-41f3-d1c6-08dc68d60510
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2024 05:26:00.3317
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7bAb57sGG0aZndMUxpR4lMLNfRctrx4tVkxpFAyEucdo9pFwNkYvW52KXG5HDqdETEbP/eLPgZmICTemhHMCzqp36jsR9uIp8q4MLOxkQNg=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5218
X-OriginatorOrg: intel.com

Verma, Vishal L wrote:
> > > @@ -560,15 +551,12 @@ static ssize_t delete_store(struct device *dev, struct device_attribute *attr,
> > > ?	if (!victim)
> > > ?		return -ENXIO;
> > > ?
> > > -	rc = down_write_killable(&dax_region_rwsem);
> > > -	if (rc)
> > > -		return rc;
> > > -	rc = down_write_killable(&dax_dev_rwsem);
> > > -	if (rc) {
> > > -		up_write(&dax_region_rwsem);
> > > -		return rc;
> > > -	}
> > > +	device_lock(dev);
> > > +	device_lock(victim);
> > > ?	dev_dax = to_dev_dax(victim);
> > > +	rc = down_write_killable(&dax_dev_rwsem);
> > 
> > This begs the question, why down_write_killable(), but not
> > device_lock_interruptible()?
> 
> Do you mean change the device_lock()s to device_lock_interruptible() in
> addition to the taking the rwsem (i.e. not instead of the rwsem..)?

I mean convert the rwsem to drop _killable.

> I guess I just restored what was there previously - but the
> interruptible variant makes sense, I can make that change.

So the original code did device_lock(), then the rework added killable
rwsem (deleted device_lock()), and now the fixes add device_lock() back.
So now that there is a mix of killable/interruptible lock usage all the
locks should agree.

Since there really is no risk of these operations being long running
there is no driving need to make them killable/interruptible, so go with
the simple option.