linux-kernel - Re: [PATCH for-next] scsi: Implement host state statistics

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]

Date:   Wed, 15 Mar 2023 07:36:19 +0100
From:   Greg KH <gregkh@...uxfoundation.org>
To:     "Seymour, Shane M" <shane.seymour@....com>
Cc:     "Martin K. Petersen" <martin.petersen@...cle.com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>
Subject: Re: [PATCH for-next] scsi: Implement host state statistics

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