From: John Garry <john.garry@huawei.com>
To: Bart Van Assche <bvanassche@acm.org>,
"Martin K . Petersen" <martin.petersen@oracle.com>
Cc: <linux-scsi@vger.kernel.org>, Christoph Hellwig <hch@lst.de>,
Ming Lei <ming.lei@redhat.com>, Hannes Reinecke <hare@suse.de>,
Mike Christie <michael.christie@oracle.com>,
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>,
"James E.J. Bottomley" <jejb@linux.ibm.com>
Subject: Re: [PATCH v5 4/7] scsi: core: Introduce a new list for SCSI proc directory entries
Date: Thu, 15 Sep 2022 11:34:08 +0100 [thread overview]
Message-ID: <1c7dade7-c821-e851-4b94-1c27ccd1fac6@huawei.com> (raw)
In-Reply-To: <20220914225621.415631-5-bvanassche@acm.org>
On 14/09/2022 23:56, Bart Van Assche wrote:
> Instead of using scsi_host_template members to track the SCSI proc
> directory entries, track these entries in a list. This patch changes the
> time needed for looking up the proc dir pointer from O(1) into O(n). I
> think this is acceptable since the number of SCSI host adapter types per
> host is usually small (less than ten).
>
> This patch has been tested by attaching two USB storage devices to a
> qemu host:
>
> $ grep -aH . /proc/scsi/usb-storage/*
> /proc/scsi/usb-storage/7: Host scsi7: usb-storage
> /proc/scsi/usb-storage/7: Vendor: QEMU
> /proc/scsi/usb-storage/7: Product: QEMU USB HARDDRIVE
> /proc/scsi/usb-storage/7:Serial Number: 1-0000:00:02.1:00.0-6
> /proc/scsi/usb-storage/7: Protocol: Transparent SCSI
> /proc/scsi/usb-storage/7: Transport: Bulk
> /proc/scsi/usb-storage/7: Quirks: SANE_SENSE
> /proc/scsi/usb-storage/8: Host scsi8: usb-storage
> /proc/scsi/usb-storage/8: Vendor: QEMU
> /proc/scsi/usb-storage/8: Product: QEMU USB HARDDRIVE
> /proc/scsi/usb-storage/8:Serial Number: 1-0000:00:02.1:00.0-7
> /proc/scsi/usb-storage/8: Protocol: Transparent SCSI
> /proc/scsi/usb-storage/8: Transport: Bulk
> /proc/scsi/usb-storage/8: Quirks: SANE_SENSE
>
> This patch prepares for constifying most SCSI host templates.
>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Ming Lei <ming.lei@redhat.com>
> Cc: Hannes Reinecke <hare@suse.de>
> Cc: John Garry <john.garry@huawei.com>
> Cc: Mike Christie <michael.christie@oracle.com>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
I have some comments below, but no big ones, so:
Reviewed-by: John Garry <john.garry@huawei.com>
> ---
> drivers/scsi/scsi_priv.h | 4 +-
> drivers/scsi/scsi_proc.c | 112 ++++++++++++++++++++++++++++++++-------
> include/scsi/scsi_host.h | 12 -----
> 3 files changed, 94 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
> index 8c2e32121db1..456ff53fe404 100644
> --- a/drivers/scsi/scsi_priv.h
> +++ b/drivers/scsi/scsi_priv.h
> @@ -100,8 +100,8 @@ extern void scsi_evt_thread(struct work_struct *work);
>
> /* scsi_proc.c */
> #ifdef CONFIG_SCSI_PROC_FS
> -extern int scsi_proc_hostdir_add(struct scsi_host_template *);
> -extern void scsi_proc_hostdir_rm(struct scsi_host_template *);
> +extern int scsi_proc_hostdir_add(const struct scsi_host_template *);
> +extern void scsi_proc_hostdir_rm(const struct scsi_host_template *);
> extern void scsi_proc_host_add(struct Scsi_Host *);
> extern void scsi_proc_host_rm(struct Scsi_Host *);
> extern int scsi_init_procfs(void);
> diff --git a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
> index 8c84f1a74773..76229ca89e2f 100644
> --- a/drivers/scsi/scsi_proc.c
> +++ b/drivers/scsi/scsi_proc.c
> @@ -43,8 +43,23 @@
>
> static struct proc_dir_entry *proc_scsi;
>
> -/* Protect sht->present and sht->proc_dir */
> +/* Protects scsi_proc_list */
> static DEFINE_MUTEX(global_host_template_mutex);
> +static LIST_HEAD(scsi_proc_list);
> +
> +/**
> + * struct scsi_proc_entry - (host template, SCSI proc dir) association
> + * @entry: entry in scsi_proc_list.
> + * @sht: SCSI host template associated with the procfs directory.
> + * @proc_dir: procfs directory associated with the SCSI host template.
> + * @present: Number of SCSI hosts instantiated for @sht.
> + */
> +struct scsi_proc_entry {
> + struct list_head entry;
> + const struct scsi_host_template *sht;
> + struct proc_dir_entry *proc_dir;
> + unsigned int present;
> +};
>
> static ssize_t proc_scsi_host_write(struct file *file, const char __user *buf,
> size_t count, loff_t *ppos)
> @@ -83,6 +98,32 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
> 4 * PAGE_SIZE);
> }
>
> +static struct scsi_proc_entry *
> +__scsi_lookup_proc_entry(const struct scsi_host_template *sht)
> +{
> + struct scsi_proc_entry *e;
> +
> + lockdep_assert_held(&global_host_template_mutex);
> +
> + list_for_each_entry(e, &scsi_proc_list, entry)
> + if (e->sht == sht)
> + return e;
> +
> + return NULL;
> +}
> +
> +static struct scsi_proc_entry *
> +scsi_lookup_proc_entry(const struct scsi_host_template *sht)
> +{
> + struct scsi_proc_entry *e;
> +
> + mutex_lock(&global_host_template_mutex);
> + e = __scsi_lookup_proc_entry(sht);
> + mutex_unlock(&global_host_template_mutex);
> +
> + return e;
> +}
> +
> /**
> * scsi_template_proc_dir() - returns the procfs dir for a SCSI host template
> * @sht: SCSI host template pointer.
> @@ -90,7 +131,9 @@ static int proc_scsi_host_open(struct inode *inode, struct file *file)
> struct proc_dir_entry *
> scsi_template_proc_dir(const struct scsi_host_template *sht)
> {
> - return sht->proc_dir;
> + struct scsi_proc_entry *e = scsi_lookup_proc_entry(sht);
> +
> + return e ? e->proc_dir : NULL;
> }
> EXPORT_SYMBOL_GPL(scsi_template_proc_dir);
>
> @@ -108,24 +151,38 @@ static const struct proc_ops proc_scsi_ops = {
> *
> * Sets sht->proc_dir to the new directory.
> */
> -int scsi_proc_hostdir_add(struct scsi_host_template *sht)
> +int scsi_proc_hostdir_add(const struct scsi_host_template *sht)
> {
> - int ret = 0;
> + struct scsi_proc_entry *e;
> + int ret = -ENOMEM;
>
> if (!sht->show_info)
> return 0;
>
> mutex_lock(&global_host_template_mutex);
> - if (!sht->present++) {
> - sht->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
> - if (!sht->proc_dir) {
> - printk(KERN_ERR "%s: proc_mkdir failed for %s\n",
> - __func__, sht->proc_name);
> - ret = -ENOMEM;
> - }
> + e = __scsi_lookup_proc_entry(sht);
> + if (!e) {
> + e = kzalloc(sizeof(*e), GFP_KERNEL);
> + if (!e)
> + goto unlock;
maybe it's better to set ret = -ENOMEM here (and not initialize ret), as
every other path it is set, AFAICS
> }
> + if (e->present++)
> + goto success;
> + e->proc_dir = proc_mkdir(sht->proc_name, proc_scsi);
> + if (!e->proc_dir) {
> + printk(KERN_ERR "%s: proc_mkdir failed for %s\n", __func__,
> + sht->proc_name);
> + goto unlock;
> + }
> + e->sht = sht;
> + list_add_tail(&e->entry, &scsi_proc_list);
> +success:
> + e = NULL;
> + ret = 0;
> +unlock:
> mutex_unlock(&global_host_template_mutex);
Note: The code might be just neater to have multiple calls to
mutex_unlock(). Maybe you have looked at that, I don't know.
>
> + kfree(e);
> return ret;
> }
>
> @@ -133,15 +190,19 @@ int scsi_proc_hostdir_add(struct scsi_host_template *sht)
> * scsi_proc_hostdir_rm - remove directory in /proc for a scsi host
> * @sht: owner of directory
> */
> -void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
> +void scsi_proc_hostdir_rm(const struct scsi_host_template *sht)
> {
> + struct scsi_proc_entry *e;
> +
> if (!sht->show_info)
> return;
>
> mutex_lock(&global_host_template_mutex);
> - if (!--sht->present && sht->proc_dir) {
> + e = __scsi_lookup_proc_entry(sht);
> + if (e && !--e->present) {
> remove_proc_entry(sht->proc_name, proc_scsi);
> - sht->proc_dir = NULL;
> + list_del(&e->entry);
> + kfree(e);
> }
> mutex_unlock(&global_host_template_mutex);
> }
> @@ -153,16 +214,21 @@ void scsi_proc_hostdir_rm(struct scsi_host_template *sht)
> */
> void scsi_proc_host_add(struct Scsi_Host *shost)
> {
> - struct scsi_host_template *sht = shost->hostt;
> + const struct scsi_host_template *sht = shost->hostt;
> + struct scsi_proc_entry *e;
> struct proc_dir_entry *p;
> char name[10];
>
> - if (!sht->proc_dir)
> + if (!sht->show_info)
> + return;
> +
> + e = scsi_lookup_proc_entry(sht);
hmm... this really should not fail, right?. Maybe an error message would
be appropiate here (for failure).
> + if (!e)
> return;
>
> sprintf(name,"%d", shost->host_no);
> - p = proc_create_data(name, S_IRUGO | S_IWUSR,
> - sht->proc_dir, &proc_scsi_ops, shost);
> + p = proc_create_data(name, S_IRUGO | S_IWUSR, e->proc_dir,
> + &proc_scsi_ops, shost);
> if (!p)
> printk(KERN_ERR "%s: Failed to register host %d in"
> "%s\n", __func__, shost->host_no,
> @@ -175,13 +241,19 @@ void scsi_proc_host_add(struct Scsi_Host *shost)
> */
> void scsi_proc_host_rm(struct Scsi_Host *shost)
> {
> + const struct scsi_host_template *sht = shost->hostt;
> + struct scsi_proc_entry *e;
> char name[10];
>
> - if (!shost->hostt->proc_dir)
> + if (!sht->show_info)
> + return;
> +
> + e = scsi_lookup_proc_entry(sht);
Same comment as scsi_proc_host_add
> + if (!e)
> return;
>
> sprintf(name,"%d", shost->host_no);
> - remove_proc_entry(name, shost->hostt->proc_dir);
> + remove_proc_entry(name, e->proc_dir);
> }
> /**
> * proc_print_scsidevice - return data about this host
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 030faca947d2..fb8184d87384 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -357,12 +357,6 @@ struct scsi_host_template {
> */
> const char *proc_name;
>
> - /*
> - * Used to store the procfs directory if a driver implements the
> - * show_info method.
> - */
> - struct proc_dir_entry *proc_dir;
> -
> /*
> * This determines if we will use a non-interrupt driven
> * or an interrupt driven scheme. It is set to the maximum number
> @@ -423,12 +417,6 @@ struct scsi_host_template {
> */
> short cmd_per_lun;
>
> - /*
> - * present contains counter indicating how many boards of this
> - * type were found when we did the scan.
> - */
> - unsigned char present;
> -
> /* If use block layer to manage tags, this is tag allocation policy */
> int tag_alloc_policy;
>
>
> .
next prev parent reply other threads:[~2022-09-15 10:34 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-14 22:56 [PATCH v5 0/7] Prepare for constifying SCSI host templates Bart Van Assche
2022-09-14 22:56 ` [PATCH v5 1/7] scsi: esas2r: Initialize two host template members implicitly Bart Van Assche
2022-09-14 22:56 ` [PATCH v5 2/7] scsi: esas2r: Introduce scsi_template_proc_dir() Bart Van Assche
2022-09-14 22:56 ` [PATCH v5 3/7] scsi: core: Fail host creation if creating the proc directory fails Bart Van Assche
2022-09-15 10:24 ` John Garry
2022-09-14 22:56 ` [PATCH v5 4/7] scsi: core: Introduce a new list for SCSI proc directory entries Bart Van Assche
2022-09-15 10:34 ` John Garry [this message]
2022-09-29 17:51 ` Bart Van Assche
2022-09-14 22:56 ` [PATCH v5 5/7] scsi: core: Fix a use-after-free related to releasing device handlers Bart Van Assche
2022-09-14 22:56 ` [PATCH v5 6/7] module: Improve support for asynchronous module exit code Bart Van Assche
2022-09-20 17:13 ` Bart Van Assche
2022-09-28 0:02 ` Luis Chamberlain
2022-09-28 18:17 ` Bart Van Assche
2022-09-30 19:39 ` Luis Chamberlain
2022-10-03 23:56 ` Luis Chamberlain
2022-10-04 0:24 ` Bart Van Assche
2022-09-28 1:09 ` Ming Lei
2022-09-28 19:27 ` Bart Van Assche
2022-09-29 1:10 ` Ming Lei
2022-09-29 17:27 ` Bart Van Assche
2022-09-14 22:56 ` [PATCH v5 7/7] scsi: core: Improve SCSI device removal Bart Van Assche
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1c7dade7-c821-e851-4b94-1c27ccd1fac6@huawei.com \
--to=john.garry@huawei.com \
--cc=bvanassche@acm.org \
--cc=hare@suse.de \
--cc=hch@lst.de \
--cc=jejb@linux.ibm.com \
--cc=krzysztof.kozlowski@linaro.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=michael.christie@oracle.com \
--cc=ming.lei@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.