Re: [PATCH 19/19] nvmet: wait only for executing I/O in preempt and abort

[Christoph Hellwig <hch@lst.de>]

Adding Guixin for the nvmet PR code.  It would have been useful
to group things a bit more..

On Thu, Oct 29, 2026 at 08:52:52AM -0700, Keith Busch wrote:
> From: Chris Mason <mason@kernel.org>
> 
> nvmet_parse_io_cmd() takes the pc_ref for an I/O command as soon as it's
> parsed, and "preempt and abort" waits for all pc_refs of the preempted
> controllers before it releases pr_sem. But a write that still needs its
> data can't complete without the host sending it and without the
> transport context receiving it, and that same context is where Register,
> Acquire and Release sleep on pr_sem. So a host that sends a write and
> holds the data back, then preempt and abort with its own key, then
> Register on the same queue, deadlocks io_work, the abort work and the
> write with each other. Two well-behaved hosts get there by accident
> when one fences the other while it re-registers under I/O. The
> connection can't be torn down, the namespace can't be disabled, and
> with nvmet_config_sem held across that, nobody can connect.
> 
> Take the pc_ref when the transport calls ->execute(), once all the data
> is there: nvmet_parse_io_cmd() stashes the real execute method and
> installs nvmet_pr_execute_io(), which gets the reference, rechecks the
> reservation access since the host may have lost it since parse time,
> and calls through. Preempt and abort then only waits for commands that
> have started executing, which need neither the host nor the transport
> to finish. A write whose data arrives after the abort now fails with
> Reservation Conflict where it was previously executed, that's the
> input the hang needs.
> 
> Fixes: 5a47c2080a73 ("nvmet: support reservation feature")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> Cc: Chaitanya Kulkarni <kch@nvidia.com>
> Signed-off-by: Chris Mason <mason@kernel.org>
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> Signed-off-by: Keith Busch <kbusch@kernel.org>
> ---
>  drivers/nvme/target/core.c  |  2 +-
>  drivers/nvme/target/nvmet.h |  4 +++-
>  drivers/nvme/target/pr.c    | 42 ++++++++++++++++++++++++++++++++++++-
>  3 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 1ed250cd4b56e..4f6fb7c4181ab 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -1176,7 +1176,7 @@ static u16 nvmet_parse_io_cmd(struct nvmet_req *req)
>  		if (ret)
>  			return ret;
>  
> -		ret = nvmet_pr_get_ns_pc_ref(req);
> +		nvmet_pr_set_execute(req);
>  	}
>  	return ret;
>  }
> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
> index 7b429cc16d491..99b3bf5cbd3b6 100644
> --- a/drivers/nvme/target/nvmet.h
> +++ b/drivers/nvme/target/nvmet.h
> @@ -513,6 +513,8 @@ struct nvmet_req {
>  	struct nvmet_port	*port;
>  
>  	void (*execute)(struct nvmet_req *req);
> +	/* what nvmet_pr_execute_io() calls once it has the pc_ref */
> +	void (*pr_execute)(struct nvmet_req *req);
>  	const struct nvmet_fabrics_ops *ops;
>  
>  	struct pci_dev		*p2p_dev;
> @@ -1041,7 +1043,7 @@ void nvmet_pr_exit_ns(struct nvmet_ns *ns);
>  void nvmet_execute_get_log_page_resv(struct nvmet_req *req);
>  u16 nvmet_set_feat_resv_notif_mask(struct nvmet_req *req, u32 mask);
>  u16 nvmet_get_feat_resv_notif_mask(struct nvmet_req *req);
> -u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req);
> +void nvmet_pr_set_execute(struct nvmet_req *req);
>  static inline void nvmet_pr_put_ns_pc_ref(struct nvmet_pr_per_ctrl_ref *pc_ref)
>  {
>  	percpu_ref_put(&pc_ref->ref);
> diff --git a/drivers/nvme/target/pr.c b/drivers/nvme/target/pr.c
> index 1a03f74c2e14f..798f5ce9011f8 100644
> --- a/drivers/nvme/target/pr.c
> +++ b/drivers/nvme/target/pr.c
> @@ -1060,7 +1060,7 @@ u16 nvmet_pr_check_cmd_access(struct nvmet_req *req)
>  	return status;
>  }
>  
> -u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
> +static u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
>  {
>  	struct nvmet_pr_per_ctrl_ref *pc_ref;
>  
> @@ -1072,6 +1072,46 @@ u16 nvmet_pr_get_ns_pc_ref(struct nvmet_req *req)
>  	return NVME_SC_SUCCESS;
>  }
>  
> +static void nvmet_pr_execute_io(struct nvmet_req *req)
> +{
> +	u16 status;
> +
> +	status = nvmet_pr_get_ns_pc_ref(req);
> +	if (!status) {
> +		/*
> +		 * The host can have lost its registration since the command was
> +		 * parsed. A get that succeeds after "preempt and abort" has
> +		 * resurrected the pc_ref has to see that. In percpu mode the
> +		 * get orders no later load, so this pairs with the
> +		 * smp_store_release() in __percpu_ref_switch_to_percpu(). While
> +		 * the resurrect is under way the get is a fully ordered
> +		 * atomic_long_inc_not_zero(). queue_work() in
> +		 * nvmet_execute_pr_acquire() orders the unregistration before
> +		 * the resurrect.
> +		 */
> +		smp_rmb();
> +		status = nvmet_pr_check_cmd_access(req);
> +	}
> +	if (status) {
> +		nvmet_req_complete(req, status);
> +		return;
> +	}
> +
> +	req->pr_execute(req);
> +}
> +
> +/*
> + * "Preempt and abort" waits for the commands that hold a pc_ref. Until the
> + * transport calls ->execute(), the end of a command can depend on the host,
> + * which may still have to send the data, and on the context and the resources
> + * of the transport. So a command gets its pc_ref only then.
> + */
> +void nvmet_pr_set_execute(struct nvmet_req *req)
> +{
> +	req->pr_execute = req->execute;
> +	req->execute = nvmet_pr_execute_io;
> +}
> +
>  static void nvmet_pr_ctrl_ns_all_cmds_done(struct percpu_ref *ref)
>  {
>  	struct nvmet_pr_per_ctrl_ref *pc_ref =
> -- 
> 2.52.0
---end quoted text---