Re: [PATCH v6 11/13] scsi: fnic: Track NVMe transport statistics

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Wait queue timeout and NVMe response metrics are declared and read, but never populated.
--

--- Patch [11]: [PATCH v6 11/13] scsi: fnic: Track NVMe transport statistics ---
commit 9923585e497ab09ae056c5f083aec96413fc2a32
Author: Karan Tilak Kumar <kartilak@cisco.com>

scsi: fnic: Track NVMe transport statistics

This commit adds counters for NVMe requests, responses, LS handling, aborts,
and wait-queue activity. It also updates NVMe I/O, completion, LS response,
LS abort, and abort paths to maintain the new counters.

Closes: https://lore.kernel.org/oe-kbuild-all/202605280619.pmobiDWp-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202605280519.Jd4fmgAZ-lkp@intel.com/

> diff --git a/drivers/scsi/fnic/fnic_nvme.c b/drivers/scsi/fnic/fnic_nvme.c
> --- a/drivers/scsi/fnic/fnic_nvme.c
> +++ b/drivers/scsi/fnic/fnic_nvme.c

[ ... ]

> @@ -1755,6 +1912,15 @@ void nvfnic_fcpio_abort(struct nvme_fc_local_port *lport,
>  		FNIC_NVME_DBG(KERN_INFO, fnic,
>  			      "cmd tag freed or not issued:0x%x sn:0x%08x\n",
>  			      io_req->tag, be32_to_cpu(cmd_iu->csn));
> +		num_ios_waitq =
> +		    atomic64_read(&fnic_stats->io_stats.nvme_num_ios_in_waitq);
> +		waitq_2sec =
> +		    atomic64_read(&fnic_stats->io_stats.nvme_ios_in_waitq_3000_msec);
> +		waitq_max_time =
> +		    atomic64_read(&fnic_stats->io_stats.nvme_ios_in_waitq_max_time);
> +		FNIC_NVME_DBG(KERN_INFO, fnic,
> +			      "waitq:%ld waitq_2sec:%ld max_wait:%ld\n",
> +			      num_ios_waitq, waitq_2sec, waitq_max_time);
>  		spin_unlock_irqrestore(&fnic->fnic_lock, flags);
>  		return;
>  	}

[ ... ]

> @@ -1855,6 +2038,7 @@ void nvfnic_nvme_iodone_work(struct work_struct *work)
>  	llnode = llist_del_all(&fnic->nvme_io_event_llist);
>  	llist_for_each_entry_safe(io_req, tmp, llnode, nvfnic_io_cmpl) {
>  		atomic_dec(&fnic->nvme_io_event_queued);
> +		atomic64_dec(&fnic->fnic_stats.io_stats.nvme_num_ios_in_waitq);
>  		io_req->fcp_req->done(io_req->fcp_req);
>  	}
>  }

[Severity: Medium]
Are we missing the logic to calculate and populate the wait queue duration 
metrics?

The nvme_ios_in_waitq_3000_msec and nvme_ios_in_waitq_max_time counters are 
read and logged in nvfnic_fcpio_abort(), but it appears they are never 
populated anywhere in the code. Should they be updated here in 
nvfnic_nvme_iodone_work() using io_req->waitq_start_time when processing 
the completion?

> diff --git a/drivers/scsi/fnic/fnic_stats.h b/drivers/scsi/fnic/fnic_stats.h
> --- a/drivers/scsi/fnic/fnic_stats.h
> +++ b/drivers/scsi/fnic/fnic_stats.h

[ ... ]

> @@ -29,6 +29,15 @@ struct io_path_stats {
>  	atomic64_t io_greater_than_30000_msec;
>  	atomic64_t current_max_io_time;
>  	atomic64_t ios[FNIC_MQ_MAX_QUEUES];
> +
> +	atomic64_t nvme_io_reqs_rcvd;
> +	atomic64_t nvme_ios_queued_for_rsp;
> +	atomic64_t nvme_io_rsps_unqueued;
> +	atomic64_t nvme_io_rsps_sending;
> +	atomic64_t nvme_io_rsps_sent;
> +	atomic64_t nvme_num_ios_in_waitq;
> +	atomic64_t nvme_ios_in_waitq_3000_msec;
> +	atomic64_t nvme_ios_in_waitq_max_time;
>  };

[Severity: Medium]
In addition to the wait queue metrics mentioned above, does this code also 
leave nvme_io_rsps_unqueued and nvme_io_rsps_sending completely unpopulated? 
They appear to be declared here but are never updated or read in the modified 
code.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724174811.5118-1-kartilak@cisco.com?part=11