Date:   Wed, 15 Mar 2023 07:36:19 +0100
From:   Greg KH <gregkh@linuxfoundation.org>
To:     "Seymour, Shane M" <shane.seymour@hpe.com>
Cc:     "Martin K. Petersen" <martin.petersen@oracle.com>,
        "jejb@linux.ibm.com" <jejb@linux.ibm.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
        "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
Subject: Re: [PATCH for-next] scsi: Implement host state statistics
Message-ID: <ZBFnYwtr+2bfjvcY@kroah.com>
References: <DM4PR84MB1373DCD07EABD28D88129C50FDBF9@DM4PR84MB1373.NAMPRD84.PROD.OUTLOOK.COM>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <DM4PR84MB1373DCD07EABD28D88129C50FDBF9@DM4PR84MB1373.NAMPRD84.PROD.OUTLOOK.COM>
Precedence: bulk

On Wed, Mar 15, 2023 at 06:08:19AM +0000, Seymour, Shane M wrote:
> The following patch implements host state statistics via sysfs. The intent
> is to allow user space to see the state changes and be able to report when
> a host changes state. The files do not separate out the time spent into
> each state but only into three:

Why does userspace care about these things at all?  What tool needs them
and what can userspace do with the information?

> 
> $ ll /sys/class/scsi_host/host0/stats
> total 0
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_first_change_time
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_last_change_time
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_other_count
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_other_ns
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_recovery_count
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_recovery_ns
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_running_count
> -r--r--r--. 1 root root 4096 Mar 13 22:43 state_running_ns
> 
> They are running, recovery and other. The running state is SHOST_CREATED
> and SHOST_RUNNING. The recovery state is SHOST_RECOVERY,
> SHOST_CANCEL_RECOVERY, and SHOST_DEL_RECOVERY. Any other state gets
> accounted for in other.
> 
> The current state is not accounted for in the information read. Because
> of that you must read:
> 
> 1. The last_change_time for that host
> 2. the current state of a host and the uptime
> 3. each of the above *count and *ns files
> 4. Re-read the last_change_time
> 5. Compare the two last_change_time values read and if different try again.
> 6. The total time read from the *ns files is subtracted from the uptime and
>    that time is then allocated to the current state time.
> 
> The first change time is to determine when the host was created so programs
> can determine if it was newly created or not.
> 
> A (GPLv2) program called hostmond will be released in a few months that
> will monitor these interfaces and report (local host only via syslog(3C))
> when hosts change state.

We kind of need to see this before the kernel changes can be accepted
for obvious reasons, what is preventing that from happening now?

> +static ssize_t state_first_change_time_show(struct device *dev,
> +	struct device_attribute *attr, char *buf)
> +{
> +	struct Scsi_Host *shost = class_to_shost(dev);
> +
> +	return scnprintf(buf, PAGE_SIZE, "%lld",
> +		ktime_to_ns(shost->stats->state_first_change_time));

Please always use sysfs_emit() instead of the crazy scnprintf() for
sysfs entries.

> +struct scsi_host_stats {
> +	ktime_t state_running_ns;
> +	ktime_t state_recovery_ns;
> +	ktime_t state_other_ns;
> +	ktime_t state_first_change_time;
> +	ktime_t state_last_change_time;
> +	uint32_t state_running_count;
> +	uint32_t state_recovery_count;
> +	uint32_t state_other_count;

u32 is a kernel type, not uint32_t please, but I don't know what the
scsi layer is used to.

thanks,

greg k-h