Received-SPF: pass (google.com: domain of linux-kernel+bounces-163225-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 29 Apr 2024 18:25:12 -0700
From: Dan Williams <dan.j.williams@intel.com>
To: Vishal Verma <vishal.l.verma@intel.com>, Dan Williams
	<dan.j.williams@intel.com>, Dave Jiang <dave.jiang@intel.com>, "Alison
 Schofield" <alison.schofield@intel.com>, Andrew Morton
	<akpm@linux-foundation.org>
CC: <linux-mm@kvack.org>, <nvdimm@lists.linux.dev>,
	<linux-cxl@vger.kernel.org>, <linux-kernel@vger.kernel.org>, Vishal Verma
	<vishal.l.verma@intel.com>
Subject: Re: [PATCH v2 2/4] dax/bus.c: fix locking for unregister_dax_dev /
 unregister_dax_mapping paths
Message-ID: <6630487879c61_148729427@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>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20240416-vv-dax_abi_fixes-v2-2-d5f0c8ec162e@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?KyTwiwpp8Xq4QI09eo/m+ea6JsBxsg3nwHB6a1IPRhQqPL26QHa2wNL2Bfap?=
 =?us-ascii?Q?CbdL2lPbyHi7YN9DsmZUvlClvNPnRyXZ0eqh7AN5tzisOxplk2W0lWcrXRoh?=
 =?us-ascii?Q?XsH8eP0yTkzGVVHXV1B2Yxd2K/Cqa3qbv8E7HzgFp+46Kt2Jf8kismcBkxsZ?=
 =?us-ascii?Q?ofkA01kXZkLuEsgb1N+jl0/8vECuXIPL20pM9NyZfYVKvJI0NcO0fvFqwNVA?=
 =?us-ascii?Q?ArP+D6TwyJ2En0amev1OYYEreWGyMnEObSaI8Lr7MCntIrZqJhtSoAY83B9h?=
 =?us-ascii?Q?A4lPR89Bc29DGaSCLt9JLkpOkT0Ge8aMMD909SS+oy6DXCC2pybZE119TmMa?=
 =?us-ascii?Q?IdllBQ8d6DWu6xXJEj7SM2wZw0fe6kppbL15Bisik6ApRY+KRFBCDZ3MUqMB?=
 =?us-ascii?Q?QU3dtK3LTOJev6dTGTWX+aSPjir2c8jPyUw/YT1BfpgKr31nQCHW5ijVOjWz?=
 =?us-ascii?Q?yw8SKhXhuapjok50trBDbqXPFXsrmYLm04PEVNIu08EcAgKc8+YT93dxxYk8?=
 =?us-ascii?Q?3YifL7YKTxSIuZPqLvgg4M1EG2svINWahrwxpbZi08i3rxcFbDhiDDHsDMEM?=
 =?us-ascii?Q?vvFGt6SpLV6TdOOf2j/K31hF0io1PjPbehE/fQ99lmBBdE4Rsk3WYFnyADHB?=
 =?us-ascii?Q?H2Vn4f/GFjgXJioFvCfBNqztvGDIZh4wYQQmFtl0j+2hkZCNKEImdtPUZLP1?=
 =?us-ascii?Q?uVcs6S/3LJ2CN11plCGMRQ1hySZeOpdjO0rgXonT9AScjLR3tR1mC1NW4Y3N?=
 =?us-ascii?Q?jAOyWGFl1u5XAE6UOmAt82Sc1r8OX7HemCrtZBUzVfbmkgd4qKzzYJar2GKi?=
 =?us-ascii?Q?jXpj2DlyOMy7DsjJEkMWhRfv9qCoVXqW+NTw5GbaGjIM7D8qjtwRLChovHWc?=
 =?us-ascii?Q?6rppqaoPrd/35XvvXi/X5PnFnDQns74kk/mKJthJlIt0ztoW97IObUIorWfH?=
 =?us-ascii?Q?PMQ3dIBU+3vZXeEvdiSxGmmAPnvFOQ1XbggA9eTAXormUTzxhM3nEyvPYu/m?=
 =?us-ascii?Q?WAki7vtA4U4tVVZ9C8Ro0dH5lc1CcmuBJ951EEY19aWnm3SKwd+GlXPz5bT5?=
 =?us-ascii?Q?PqQLCJjYTIZEcDmhYknH6xDyc+DE6GvJ/4gnzHRwcHCuXknxetcizsbkrqMF?=
 =?us-ascii?Q?4F7nYiHkK0sqT/9Kb01gsOo/aJmH6cx5CVLBWugbKTs02pMzsFu0eRITnvr1?=
 =?us-ascii?Q?IuiGg6L47kwuD1b+xUXmkk/DIknvTZi9LMFPF2m5uJnhMW4MwAgd/G7vKe5Z?=
 =?us-ascii?Q?BVgmQiTYQhjddQM3RW9a9wf34ujkE/HT5lhpZ5kaC1c38bJeYuvIezebLdwT?=
 =?us-ascii?Q?//16yV0Hr44fJ/0ZdOrbOnYLVN2V+/UOmyUfsyNVODNuIZ5HfpBQeTFDg4DD?=
 =?us-ascii?Q?CQfeDem/MdBjVkiuqvSnVlLHbC0+4rReoy+m7MC2anxTNwxHhp+Vc6ZiLIi2?=
 =?us-ascii?Q?qPcYR0rFJw/bJPf3y2eEXNykBjguoeWwgnlB8gGuJC1npfMyHlgG63Fg/Wk+?=
 =?us-ascii?Q?+Y83Udj7hEbIhIvy8UJRP6i7wFx84HIRERH+37+EuIKdh377nLLBFCboe393?=
 =?us-ascii?Q?pk5SGeuvbDk9Gcsj6f1o3BXq/JfZJ4xhirPeDOISCNQyv62UpiGkHHQwxr4l?=
 =?us-ascii?Q?Ng=3D=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: a1d89ae9-95d5-4c9a-2909-08dc68b4633c
X-MS-Exchange-CrossTenant-AuthSource: PH8PR11MB8107.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 30 Apr 2024 01:25:14.9814
 (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: O/xbv/ygoo3XC2IzTQ4Ys7Yxroo1VTsy5BizWR8eacoB4bhYBLR13NR6/a3hIpCias1T54Sq4i9B5W3KZ2Mf50gDB9WtDFSXnEAAQBZFTT0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR11MB7351
X-OriginatorOrg: intel.com

Vishal Verma wrote:
> Commit c05ae9d85b47 ("dax/bus.c: replace driver-core lock usage by a local rwsem")
> was a bit overzealous in eliminating device_lock() usage, and ended up
> removing a couple of lock acquisitions which were needed, and as a
> result, fix some of the conditional locking missteps that the above
> commit introduced in unregister_dax_dev() and unregister_dax_mapping().

I think it makes sense to tell the story a bit about why the
delete_store() conversion was problematic, because the
unregister_dev_dax() changes were just a knock-on effect to fixing the
delete_store() flow.

Something like:

---
commit c05ae9d85b47 ("dax/bus.c: replace driver-core lock usage by a local rwsem")
aimed to undo device_lock() abuses for protecting changes to dax-driver
internal data-structures like the dax_region resource tree to
device-dax-instance range structures. However, the device_lock() was legitamately
enforcing that devices to be deleted were not current actively attached
to any driver nor assigned any capacity from the region.
---

..you can fill in a couple notes about the knock-on fixups after that
was restored.

> Fixes: c05ae9d85b47 ("dax/bus.c: replace driver-core lock usage by a local rwsem")
> Reported-by: Dan Williams <dan.j.williams@intel.com>
> Signed-off-by: Vishal Verma <vishal.l.verma@intel.com>
> ---
>  drivers/dax/bus.c | 44 ++++++++++----------------------------------
>  1 file changed, 10 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/dax/bus.c b/drivers/dax/bus.c
> index 7924dd542a13..4e04b228b080 100644
> --- a/drivers/dax/bus.c
> +++ b/drivers/dax/bus.c
> @@ -465,26 +465,17 @@ static void free_dev_dax_ranges(struct dev_dax *dev_dax)
>  		trim_dev_dax_range(dev_dax);
>  }
>  
> -static void __unregister_dev_dax(void *dev)
> +static void unregister_dev_dax(void *dev)
>  {
>  	struct dev_dax *dev_dax = to_dev_dax(dev);
>  
>  	dev_dbg(dev, "%s\n", __func__);
>  
> +	down_write(&dax_region_rwsem);
>  	kill_dev_dax(dev_dax);
>  	device_del(dev);
>  	free_dev_dax_ranges(dev_dax);
>  	put_device(dev);
> -}
> -
> -static void unregister_dev_dax(void *dev)
> -{
> -	if (rwsem_is_locked(&dax_region_rwsem))
> -		return __unregister_dev_dax(dev);
> -
> -	if (WARN_ON_ONCE(down_write_killable(&dax_region_rwsem) != 0))
> -		return;
> -	__unregister_dev_dax(dev);
>  	up_write(&dax_region_rwsem);
>  }
>  
> @@ -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()?

I do not expect any of this is long running so likely down_write() is
sufficient here, especially since the heaviest locks to acquire are
already held by the time rwsem is considered.

Other than that this looks good to me:

You can include my Reviewed-by on the next posting.