linux-kernel - Re: [PATCH 1/1] nvme-multipath: Skip nr_active increments in RETRY disposition

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] [day] [month] [year] [list]

Message-ID: <20250925011427.GC3269-mkhalfella@purestorage.com>
Date: Wed, 24 Sep 2025 18:14:27 -0700
From: Mohamed Khalfella <mkhalfella@...estorage.com>
To: Keith Busch <kbusch@...nel.org>
Cc: Amit Chaudhary <achaudhary@...estorage.com>,
	Jens Axboe <axboe@...nel.dk>, Christoph Hellwig <hch@....de>,
	Sagi Grimberg <sagi@...mberg.me>, randyj@...estorage.com,
	jmeneghi@...hat.com, emilne@...hat.com,
	linux-nvme@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/1] nvme-multipath: Skip nr_active increments in RETRY
 disposition

On 2025-09-24 17:02:51 -0600, Keith Busch wrote:
> On Wed, Sep 24, 2025 at 03:43:18PM -0700, Amit Chaudhary wrote:
> >  static inline void nvme_start_request(struct request *rq)
> >  {
> > -	if (rq->cmd_flags & REQ_NVME_MPATH)
> > +	if ((rq->cmd_flags & REQ_NVME_MPATH) && (!nvme_req(rq)->retries))
> >  		nvme_mpath_start_request(rq);
> >  	blk_mq_start_request(rq);
> >  }
> 
> Using "retries" is bit indirect as a proxy for multipath active counts.
> Could this be moved to the mpath start instead, directly using the flag
> that accounts for the path? This also helps to keep track if the command
> gets retried across a user toggling the policy to "qd".
> 
> ---
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 3da980dc60d91..1c630967ddd40 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
> @@ -182,7 +182,8 @@ void nvme_mpath_start_request(struct request *rq)
>         struct nvme_ns *ns = rq->q->queuedata;
>         struct gendisk *disk = ns->head->disk;
> 
> -       if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD) {
> +       if (READ_ONCE(ns->head->subsys->iopolicy) == NVME_IOPOLICY_QD &&
> +           !(nvme_req(rq)->flags & NVME_MPATH_CNT_ACTIVE)) {
>                 atomic_inc(&ns->ctrl->nr_active);
>                 nvme_req(rq)->flags |= NVME_MPATH_CNT_ACTIVE;
>         }
> --

193         nvme_req(rq)->flags |= NVME_MPATH_IO_STATS;
194         nvme_req(rq)->start_time = bdev_start_io_acct(disk->part0, req_op(rq),
195                                                       jiffies);

Doing it this way might messup with stats accounting because the two
lines above will be executed on request retry. I do not think we need
that, right?