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
Date: Mon, 29 Apr 2024 18:25:12 -0700 [thread overview]
Message-ID: <6630487879c61_148729427@dwillia2-mobl3.amr.corp.intel.com.notmuch> (raw)
In-Reply-To: <20240416-vv-dax_abi_fixes-v2-2-d5f0c8ec162e@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.
next prev parent reply other threads:[~2024-04-30 1:25 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-16 21:46 [PATCH v2 0/4] dax/bus.c: Fixups for dax-bus locking Vishal Verma
2024-04-16 21:46 ` [PATCH v2 1/4] dax/bus.c: replace WARN_ON_ONCE() with lockdep asserts Vishal Verma
2024-04-30 1:23 ` Dan Williams
2024-04-16 21:46 ` [PATCH v2 2/4] dax/bus.c: fix locking for unregister_dax_dev / unregister_dax_mapping paths Vishal Verma
2024-04-30 1:25 ` Dan Williams [this message]
2024-04-30 4:11 ` Verma, Vishal L
2024-04-30 5:25 ` Dan Williams
2024-04-16 21:46 ` [PATCH v2 3/4] dax/bus.c: Don't use down_write_killable for non-user processes Vishal Verma
2024-04-30 1:25 ` Dan Williams
2024-04-16 21:46 ` [PATCH v2 4/4] dax/bus.c: Use the right locking mode (read vs write) in size_show Vishal Verma
2024-04-30 1:26 ` Dan Williams
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6630487879c61_148729427@dwillia2-mobl3.amr.corp.intel.com.notmuch \
--to=dan.j.williams@intel.com \
--cc=akpm@linux-foundation.org \
--cc=alison.schofield@intel.com \
--cc=dave.jiang@intel.com \
--cc=linux-cxl@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=nvdimm@lists.linux.dev \
--cc=vishal.l.verma@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.