[PATCHv2] nvme: use ctrl state accessor

[PATCHv2] nvme: use ctrl state accessor

[Keith Busch]

From: Keith Busch <kbusch@kernel.org>

The ctrl->state value is updated in another thread using WRITE_ONCE, so
ensure all the readers use the appropriate accessor.

Signed-off-by: Keith Busch <kbusch@kernel.org>
---
v1->v2:

  Fixed error in multipath (Chaitanya)

 drivers/nvme/host/auth.c      |  2 +-
 drivers/nvme/host/fabrics.h   |  8 +++++---
 drivers/nvme/host/multipath.c | 15 ++++++++-------
 drivers/nvme/host/sysfs.c     |  6 +++---
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/auth.c b/drivers/nvme/host/auth.c
index 72c0525c75f50..2a12ee878783f 100644
--- a/drivers/nvme/host/auth.c
+++ b/drivers/nvme/host/auth.c
@@ -897,7 +897,7 @@ static void nvme_ctrl_auth_work(struct work_struct *work)
 	 * If the ctrl is no connected, bail as reconnect will handle
 	 * authentication.
 	 */
-	if (ctrl->state != NVME_CTRL_LIVE)
+	if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE)
 		return;
 
 	/* Authenticate admin queue first */
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index fbaee5a7be196..06cc54851b1be 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -185,9 +185,11 @@ static inline bool
 nvmf_ctlr_matches_baseopts(struct nvme_ctrl *ctrl,
 			struct nvmf_ctrl_options *opts)
 {
-	if (ctrl->state == NVME_CTRL_DELETING ||
-	    ctrl->state == NVME_CTRL_DELETING_NOIO ||
-	    ctrl->state == NVME_CTRL_DEAD ||
+	enum nvme_ctrl_state state = nvme_ctrl_state(ctrl);
+
+	if (state == NVME_CTRL_DELETING ||
+	    state == NVME_CTRL_DELETING_NOIO ||
+	    state == NVME_CTRL_DEAD ||
 	    strcmp(opts->subsysnqn, ctrl->opts->subsysnqn) ||
 	    strcmp(opts->host->nqn, ctrl->opts->host->nqn) ||
 	    !uuid_equal(&opts->host->id, &ctrl->opts->host->id))
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 2dd4137a08b28..74de1e64aeead 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -156,7 +156,7 @@ void nvme_kick_requeue_lists(struct nvme_ctrl *ctrl)
 		if (!ns->head->disk)
 			continue;
 		kblockd_schedule_work(&ns->head->requeue_work);
-		if (ctrl->state == NVME_CTRL_LIVE)
+		if (nvme_ctrl_state(ns->ctrl) == NVME_CTRL_LIVE)
 			disk_uevent(ns->head->disk, KOBJ_CHANGE);
 	}
 	up_read(&ctrl->namespaces_rwsem);
@@ -223,13 +223,14 @@ void nvme_mpath_revalidate_paths(struct nvme_ns *ns)
 
 static bool nvme_path_is_disabled(struct nvme_ns *ns)
 {
+	enum nvme_ctrl_state state = nvme_ctrl_state(ns->ctrl);
+
 	/*
 	 * We don't treat NVME_CTRL_DELETING as a disabled path as I/O should
 	 * still be able to complete assuming that the controller is connected.
 	 * Otherwise it will fail immediately and return to the requeue list.
 	 */
-	if (ns->ctrl->state != NVME_CTRL_LIVE &&
-	    ns->ctrl->state != NVME_CTRL_DELETING)
+	if (state != NVME_CTRL_LIVE && state != NVME_CTRL_DELETING)
 		return true;
 	if (test_bit(NVME_NS_ANA_PENDING, &ns->flags) ||
 	    !test_bit(NVME_NS_READY, &ns->flags))
@@ -331,7 +332,7 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head,
 
 static inline bool nvme_path_is_optimized(struct nvme_ns *ns)
 {
-	return ns->ctrl->state == NVME_CTRL_LIVE &&
+	return nvme_ctrl_state(ns->ctrl) == NVME_CTRL_LIVE &&
 		ns->ana_state == NVME_ANA_OPTIMIZED;
 }
 
@@ -358,7 +359,7 @@ static bool nvme_available_path(struct nvme_ns_head *head)
 	list_for_each_entry_rcu(ns, &head->list, siblings) {
 		if (test_bit(NVME_CTRL_FAILFAST_EXPIRED, &ns->ctrl->flags))
 			continue;
-		switch (ns->ctrl->state) {
+		switch (nvme_ctrl_state(ns->ctrl)) {
 		case NVME_CTRL_LIVE:
 		case NVME_CTRL_RESETTING:
 		case NVME_CTRL_CONNECTING:
@@ -667,7 +668,7 @@ static void nvme_update_ns_ana_state(struct nvme_ana_group_desc *desc,
 	 * controller is ready.
 	 */
 	if (nvme_state_is_live(ns->ana_state) &&
-	    ns->ctrl->state == NVME_CTRL_LIVE)
+	    nvme_ctrl_state(ns->ctrl) == NVME_CTRL_LIVE)
 		nvme_mpath_set_live(ns);
 }
 
@@ -748,7 +749,7 @@ static void nvme_ana_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl = container_of(work, struct nvme_ctrl, ana_work);
 
-	if (ctrl->state != NVME_CTRL_LIVE)
+	if (nvme_ctrl_state(ctrl) != NVME_CTRL_LIVE)
 		return;
 
 	nvme_read_ana_log(ctrl);
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 754e911110420..6b2f06f190de3 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -311,6 +311,7 @@ static ssize_t nvme_sysfs_show_state(struct device *dev,
 				     char *buf)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	unsigned state = (unsigned)nvme_ctrl_state(ctrl);
 	static const char *const state_name[] = {
 		[NVME_CTRL_NEW]		= "new",
 		[NVME_CTRL_LIVE]	= "live",
@@ -321,9 +322,8 @@ static ssize_t nvme_sysfs_show_state(struct device *dev,
 		[NVME_CTRL_DEAD]	= "dead",
 	};
 
-	if ((unsigned)ctrl->state < ARRAY_SIZE(state_name) &&
-	    state_name[ctrl->state])
-		return sysfs_emit(buf, "%s\n", state_name[ctrl->state]);
+	if (state < ARRAY_SIZE(state_name) && state_name[state])
+		return sysfs_emit(buf, "%s\n", state_name[state]);
 
 	return sysfs_emit(buf, "unknown state\n");
 }
-- 
2.34.1

Re: [PATCHv2] nvme: use ctrl state accessor

[Chaitanya Kulkarni]

Sven,

On 1/25/24 07:52, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The ctrl->state value is updated in another thread using WRITE_ONCE, so
> ensure all the readers use the appropriate accessor.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>

Based on this patch nvme/host/apple.c might need something like this, please
have a look at [1].

-ck

[1] Only use for discussion, don't apply or use this patch :-

diff --git a/drivers/nvme/host/apple.c b/drivers/nvme/host/apple.c
index 596bb11eeba5..2524c9a13ad7 100644
--- a/drivers/nvme/host/apple.c
+++ b/drivers/nvme/host/apple.c
@@ -797,6 +797,7 @@ static int apple_nvme_init_request(struct 
blk_mq_tag_set *set,

  static void apple_nvme_disable(struct apple_nvme *anv, bool shutdown)
  {
+       enum nvme_ctrl_state state = nvme_ctrl_state(&anv->ctrl);
         u32 csts = readl(anv->mmio_nvme + NVME_REG_CSTS);
         bool dead = false, freeze = false;
         unsigned long flags;
@@ -808,8 +809,7 @@ static void apple_nvme_disable(struct apple_nvme 
*anv, bool shutdown)
         if (csts & NVME_CSTS_CFS)
                 dead = true;

-       if (anv->ctrl.state == NVME_CTRL_LIVE ||
-           anv->ctrl.state == NVME_CTRL_RESETTING) {
+       if (state == NVME_CTRL_LIVE || state == NVME_CTRL_RESETTING) {
                 freeze = true;
                 nvme_start_freeze(&anv->ctrl);
         }
@@ -881,7 +881,7 @@ static enum blk_eh_timer_return 
apple_nvme_timeout(struct request *req)
         unsigned long flags;
         u32 csts = readl(anv->mmio_nvme + NVME_REG_CSTS);

-       if (anv->ctrl.state != NVME_CTRL_LIVE) {
+       if (nvme_ctrl_state(&anv->ctrl) != NVME_CTRL_LIVE) {
                 /*
                  * From rdma.c:
                  * If we are resetting, connecting or deleting we should
@@ -985,10 +985,11 @@ static void apple_nvme_reset_work(struct 
work_struct *work)
         u32 boot_status, aqa;
         struct apple_nvme *anv =
                 container_of(work, struct apple_nvme, ctrl.reset_work);
+       enum nvme_ctrl_state state = nvme_ctrl_state(&anv->ctrl);

-       if (anv->ctrl.state != NVME_CTRL_RESETTING) {
+       if (state != NVME_CTRL_RESETTING) {
                 dev_warn(anv->dev, "ctrl state %d is not RESETTING\n",
-                        anv->ctrl.state);
+                        state);
                 ret = -ENODEV;
                 goto out;
         }

Re: [PATCHv2] nvme: use ctrl state accessor

[Sven Peter]

Hi Chaitanya,

Sorry for the late reply, I've been pretty busy since the winter break and
only been able to catch up on emails now.

On Fri, Jan 26, 2024, at 08:20, Chaitanya Kulkarni wrote:
>
> Based on this patch nvme/host/apple.c might need something like this, please
> have a look at [1].

Thanks for noticing and catching this, that patch looks good to me :)
Looks like Keith already pick this up in a follow up in 6d3c7fb17b4 a few days ago.


Best,


Sven

Re: [PATCHv2] nvme: use ctrl state accessor

[Chaitanya Kulkarni]

On 1/25/24 07:52, Keith Busch wrote:
> From: Keith Busch <kbusch@kernel.org>
>
> The ctrl->state value is updated in another thread using WRITE_ONCE, so
> ensure all the readers use the appropriate accessor.
>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>

Keith,

Not sure if nvme_check_ready() needs nvme_ctrl_state() ?

consider :-

* Thread A running timeout handler :-
nvme_timeout()
  ends up resetting ctrl state at the end of function using WRITE_ONCE() :-

  disable:
            nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING)
             WRITE_ONCE()

* Thread B issuing I/O :-
nvme_queue_rq() or nvme_prep_rq_batch()
  nvme_check_ready()
   ctrl->state == NVME_CTRL_LIVE

Since timeout handler's nvme_change_ctrl_state() call will result in
updating ctrl->state with WRITE_ONCE(), hence reader in the 
nvme_check_ready()
should also read using nvme_ctrl_state()->READ_ONCE() ?

if it is not needed please ignore above question, looks good to me ..

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCHv2] nvme: use ctrl state accessor

[Keith Busch]

On Fri, Jan 26, 2024 at 08:25:38AM +0000, Chaitanya Kulkarni wrote:
> On 1/25/24 07:52, Keith Busch wrote:
> > From: Keith Busch <kbusch@kernel.org>
> >
> > The ctrl->state value is updated in another thread using WRITE_ONCE, so
> > ensure all the readers use the appropriate accessor.
> >
> > Signed-off-by: Keith Busch <kbusch@kernel.org>
> > ---
> >
> 
> Keith,
> 
> Not sure if nvme_check_ready() needs nvme_ctrl_state() ?
> 
> consider :-
> 
> * Thread A running timeout handler :-
> nvme_timeout()
>   ends up resetting ctrl state at the end of function using WRITE_ONCE() :-
> 
>   disable:
>             nvme_change_ctrl_state(&dev->ctrl, NVME_CTRL_RESETTING)
>              WRITE_ONCE()
> 
> * Thread B issuing I/O :-
> nvme_queue_rq() or nvme_prep_rq_batch()
>   nvme_check_ready()
>    ctrl->state == NVME_CTRL_LIVE
> 
> Since timeout handler's nvme_change_ctrl_state() call will result in
> updating ctrl->state with WRITE_ONCE(), hence reader in the 
> nvme_check_ready()
> should also read using nvme_ctrl_state()->READ_ONCE() ?
> 
> if it is not needed please ignore above question, looks good to me ..

It should be a READ_ONCE() here. Consider it's an inline function that's
called in a loop (at least from pci), so a compiler may optimize the
read so that it happens just once instead of at each iteration as
expected.

I missed this one, I'll get it added for the next version.