linux-kernel - Re: [PATCH RFC 3/3] nvme: delay failover by command quiesce timeout

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6f0d50b2-7a16-4298-8129-c3a0b1426d26@flourine.local>
Date: Tue, 15 Apr 2025 14:17:48 +0200
From: Daniel Wagner <dwagner@...e.de>
To: Mohamed Khalfella <mkhalfella@...estorage.com>
Cc: Daniel Wagner <wagi@...nel.org>, Christoph Hellwig <hch@....de>, 
	Sagi Grimberg <sagi@...mberg.me>, Keith Busch <kbusch@...nel.org>, Hannes Reinecke <hare@...e.de>, 
	John Meneghini <jmeneghi@...hat.com>, randyj@...estorage.com, linux-nvme@...ts.infradead.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC 3/3] nvme: delay failover by command quiesce timeout

On Thu, Apr 10, 2025 at 01:51:37AM -0700, Mohamed Khalfella wrote:
> > +void nvme_schedule_failover(struct nvme_ctrl *ctrl)
> > +{
> > +	unsigned long delay;
> > +
> > +	if (ctrl->cqt)
> > +		delay = msecs_to_jiffies(ctrl->cqt);
> > +	else
> > +		delay = ctrl->kato * HZ;
> 
> I thought that delay = m * ctrl->kato + ctrl->cqt
> where m = ctrl->ctratt & NVME_CTRL_ATTR_TBKAS ? 3 : 2
> no?

The failover schedule delay is the additional amount of time we have to
wait for the target to cleanup (CQT). If the CTQ is not valid I thought
the spec said to wait for a KATO. Possible I got that wrong.

The factor 3 or 2 is relavant for the timeout value for the KATO command
we schedule. The failover schedule timeout is ontop of the command
timeout value.

> > --- a/drivers/nvme/host/multipath.c
> > +++ b/drivers/nvme/host/multipath.c
> > @@ -86,9 +86,11 @@ void nvme_mpath_start_freeze(struct nvme_subsystem *subsys)
> >  void nvme_failover_req(struct request *req)
> >  {
> >  	struct nvme_ns *ns = req->q->queuedata;
> > +	struct nvme_ctrl *ctrl = nvme_req(req)->ctrl;
> >  	u16 status = nvme_req(req)->status & NVME_SCT_SC_MASK;
> >  	unsigned long flags;
> >  	struct bio *bio;
> > +	enum nvme_ctrl_state state = nvme_ctrl_state(ctrl);
> >  
> >  	nvme_mpath_clear_current_path(ns);
> >  
> > @@ -121,9 +123,53 @@ void nvme_failover_req(struct request *req)
> >  	blk_steal_bios(&ns->head->requeue_list, req);
> >  	spin_unlock_irqrestore(&ns->head->requeue_lock, flags);
> >  
> > -	nvme_req(req)->status = 0;
> > -	nvme_end_req(req);
> > -	kblockd_schedule_work(&ns->head->requeue_work);
> > +	spin_lock_irqsave(&ctrl->lock, flags);
> > +	list_add_tail(&req->queuelist, &ctrl->failover_list);
> > +	spin_unlock_irqrestore(&ctrl->lock, flags);
> 
> I see this is the only place where held requests are added to
> failover_list.
> 
> - Will this hold admin requests in failover_list?

Yes.

> - What about requests that do not go through nvme_failover_req(), like
>   passthrough requests, do we not want to hold these requests until it
>   is safe for them to be retried?

Pasthrough commands should fail immediately. Userland is in charge here,
not the kernel. At least this what should happen here.

> - In case of controller reset or delete if nvme_disable_ctrl()
>   successfully disables the controller, then we do not want to add
>   canceled requests to failover_list, right? Does this implementation
>   consider this case?

Not sure. I've tested a few things but I am pretty sure this RFC is far
from being complete.