Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Mon, 4 Dec 2023 11:41:39 +0100
From:   Daniel Wagner <dwagner@suse.de>
To:     Christoph Hellwig <hch@lst.de>
Cc:     linux-nvme@lists.infradead.org, linux-kernel@vger.kernel.org,
        Keith Busch <kbusch@kernel.org>,
        Sagi Grimberg <sagi@grimberg.me>,
        Hannes Reinecke <hare@suse.de>
Subject: Re: [RFC v2 2/3] nvme: move ns id info to struct nvme_ns_head
Message-ID: <27uglzkgku6qeaaodmyb3sudajrkibjiapsg4hjuxy57hohauv@hnpb5zbedyrv>
References: <20231201092735.28592-1-dwagner@suse.de>
 <20231201092735.28592-3-dwagner@suse.de>
 <20231204075134.GB29377@lst.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231204075134.GB29377@lst.de>
Precedence: bulk

On Mon, Dec 04, 2023 at 08:51:34AM +0100, Christoph Hellwig wrote:
> > +static void nvme_set_ref_tag(struct nvme_ns_head *head, struct nvme_command *cmnd,
> 
> .. and here.  I'm going to stop now, please also fix up all other
> places.

Sure, I'll update the patch accordingly.

> >  void nvme_failover_req(struct request *req)
> >  {
> > -	struct nvme_ns *ns = req->q->queuedata;
> > +	struct nvme_ns_head *head = req->q->queuedata;
> > +	struct nvme_ctrl *ctrl = nvme_req(req)->ctrl;
> > +	struct nvme_ns *ns;
> >  	u16 status = nvme_req(req)->status & 0x7ff;
> >  	unsigned long flags;
> >  	struct bio *bio;
> >  
> > -	nvme_mpath_clear_current_path(ns);
> > +	nvme_mpath_clear_current_path(head);
> >  
> >  	/*
> >  	 * If we got back an ANA error, we know the controller is alive but not
> >  	 * ready to serve this namespace.  Kick of a re-read of the ANA
> >  	 * information page, and just try any other available path for now.
> >  	 */
> > -	if (nvme_is_ana_error(status) && ns->ctrl->ana_log_buf) {
> > +	if (nvme_is_ana_error(status) && ctrl->ana_log_buf) {
> > +		ns = nvme_find_get_ns(ctrl, head->ns_id);
> 
> This looks unrelated.

The problem I try to address here is, that we need the ns pointer to
access the ns->flags for ANA state. Given that
nvme_mpath_clear_current_path really wants the ns pointer as well, we
need something like this at the beginning of this function.

As I said, I didn't find any other way to get get from the head pointer
to the ns pointer and this function is the only place where this is
actually necessary to do (avoiding the list scan in nvme_find_get_ns).

> 
> > -bool nvme_mpath_clear_current_path(struct nvme_ns *ns)
> > +bool nvme_mpath_clear_current_path(struct nvme_ns_head *head)
> >  {
> > -	struct nvme_ns_head *head = ns->head;
> >  	bool changed = false;
> >  	int node;
> >  
> > @@ -181,7 +183,7 @@ bool nvme_mpath_clear_current_path(struct nvme_ns *ns)
> >  		goto out;
> >  
> >  	for_each_node(node) {
> > -		if (ns == rcu_access_pointer(head->current_path[node])) {
> > +		if (head == rcu_access_pointer(head->current_path[node])->head) {
> 
> and this can't work.  We need to check the actual ns for the path
> selection, as that's kindof the point.

Okay, makes sense. I'll drop this.

Thanks,
Daniel