[PATCH] scsi: iterate over devices individually for /proc/scsi/scsi

[PATCH] scsi: iterate over devices individually for /proc/scsi/scsi

[Jeff Mahoney]

 On systems with very large numbers (> 1600 or so) of SCSI devices,
 cat /proc/scsi/scsi ends up failing with -ENOMEM. This is due to
 the show routine simply iterating over all of the devices with
 bus_for_each_dev(), and trying to dump all of them into the buffer
 at the same time. On my test system (using scsi_debug with 4064 devices),
 the output ends up being ~ 632k, far more than kmalloc will typically allow.

 This patch uses seq_file directly instead of single_file, and breaks up
 the operations into the 4 seq_file callbacks. The result is that
 each show() operation only dumps ~ 180 bytes into the buffer at a time
 so we don't run out of memory.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>

---

 drivers/scsi/scsi_proc.c |   69 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 63 insertions(+), 6 deletions(-)

diff -rup a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
--- a/drivers/scsi/scsi_proc.c	2007-04-12 13:41:06.000000000 -0400
+++ b/drivers/scsi/scsi_proc.c	2007-04-12 13:47:38.000000000 -0400
@@ -294,20 +294,77 @@ static ssize_t proc_scsi_write(struct fi
 	return err;
 }
 
-static int proc_scsi_show(struct seq_file *s, void *p)
+static struct device *next_device(struct klist_iter *i)
 {
-	seq_printf(s, "Attached devices:\n");
-	bus_for_each_dev(&scsi_bus_type, NULL, s, proc_print_scsidevice);
-	return 0;
+	struct klist_node *n = klist_next(i);
+	return n ? container_of(n, struct device, knode_bus) : NULL;
+}
+
+static void *scsi_seq_start(struct seq_file *sfile, loff_t *pos)
+{
+	struct klist_iter *iter;
+	struct device *dev = NULL;
+	loff_t l = *pos;
+
+	iter = kmalloc(sizeof (*iter), GFP_KERNEL);
+	if (!iter)
+		return ERR_PTR(-ENOMEM);
+
+	klist_iter_init_node(&scsi_bus_type.klist_devices, iter, NULL);
+
+	do {
+		dev = next_device(iter);
+	} while (l-- && dev);
+
+	sfile->private = iter;
+	return dev;
 }
 
+static void *scsi_seq_next(struct seq_file *sfile, void *v, loff_t *pos)
+{
+	struct klist_iter *iter = (struct klist_iter *)sfile->private;
+	++*pos;
+	return next_device(iter);
+}
+
+static void scsi_seq_stop(struct seq_file *sfile, void *v)
+{
+	struct klist_iter *iter = (struct klist_iter *)sfile->private;
+	sfile->private = NULL;
+	klist_iter_exit(iter);
+	kfree(iter);
+}
+
+static int scsi_seq_show(struct seq_file *sfile, void *v)
+{
+	struct klist_iter *iter = (struct klist_iter *)sfile->private;
+	struct device *dev = (struct device *)v;
+	struct klist_node *head;
+
+	spin_lock(&iter->i_klist->k_lock);
+	head = container_of(iter->i_klist->k_list.next,
+	                    struct klist_node, n_node);
+	if (&dev->knode_bus == head)
+		seq_puts(sfile, "Attached devices:\n");
+	spin_unlock(&iter->i_klist->k_lock);
+
+	return proc_print_scsidevice(dev, sfile);
+}
+
+static struct seq_operations scsi_seq_ops = {
+	.start	= scsi_seq_start,
+	.next	= scsi_seq_next,
+	.stop	= scsi_seq_stop,
+	.show	= scsi_seq_show
+};
+
 static int proc_scsi_open(struct inode *inode, struct file *file)
 {
 	/*
 	 * We don't really needs this for the write case but it doesn't
 	 * harm either.
 	 */
-	return single_open(file, proc_scsi_show, NULL);
+	return seq_open(file, &scsi_seq_ops);
 }
 
 static struct file_operations proc_scsi_operations = {
@@ -315,7 +372,7 @@ static struct file_operations proc_scsi_
 	.read		= seq_read,
 	.write		= proc_scsi_write,
 	.llseek		= seq_lseek,
-	.release	= single_release,
+	.release	= seq_release,
 };
 
 int __init scsi_init_procfs(void)

Re: [PATCH] scsi: iterate over devices individually for /proc/scsi/scsi

[Jeff Mahoney]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Jeff Mahoney wrote:
>  On systems with very large numbers (> 1600 or so) of SCSI devices,
>  cat /proc/scsi/scsi ends up failing with -ENOMEM. This is due to
>  the show routine simply iterating over all of the devices with
>  bus_for_each_dev(), and trying to dump all of them into the buffer
>  at the same time. On my test system (using scsi_debug with 4064 devices),
>  the output ends up being ~ 632k, far more than kmalloc will typically allow.
> 
>  This patch uses seq_file directly instead of single_file, and breaks up
>  the operations into the 4 seq_file callbacks. The result is that
>  each show() operation only dumps ~ 180 bytes into the buffer at a time
>  so we don't run out of memory.
> 
> Signed-off-by: Jeff Mahoney <jeffm@suse.com>
> 
> ---
> 
>  drivers/scsi/scsi_proc.c |   69 ++++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 63 insertions(+), 6 deletions(-)


I thought I'd address one of the most obvious concerns with this patch
before it comes up. I'm not particularly thrilled about open coding the
klist stuff. I'm talking with a few people on expanding the interface to
make this not suck so bad.

- -Jeff

> diff -rup a/drivers/scsi/scsi_proc.c b/drivers/scsi/scsi_proc.c
> --- a/drivers/scsi/scsi_proc.c	2007-04-12 13:41:06.000000000 -0400
> +++ b/drivers/scsi/scsi_proc.c	2007-04-12 13:47:38.000000000 -0400
> @@ -294,20 +294,77 @@ static ssize_t proc_scsi_write(struct fi
>  	return err;
>  }
>  
> -static int proc_scsi_show(struct seq_file *s, void *p)
> +static struct device *next_device(struct klist_iter *i)
>  {
> -	seq_printf(s, "Attached devices:\n");
> -	bus_for_each_dev(&scsi_bus_type, NULL, s, proc_print_scsidevice);
> -	return 0;
> +	struct klist_node *n = klist_next(i);
> +	return n ? container_of(n, struct device, knode_bus) : NULL;
> +}
> +
> +static void *scsi_seq_start(struct seq_file *sfile, loff_t *pos)
> +{
> +	struct klist_iter *iter;
> +	struct device *dev = NULL;
> +	loff_t l = *pos;
> +
> +	iter = kmalloc(sizeof (*iter), GFP_KERNEL);
> +	if (!iter)
> +		return ERR_PTR(-ENOMEM);
> +
> +	klist_iter_init_node(&scsi_bus_type.klist_devices, iter, NULL);
> +
> +	do {
> +		dev = next_device(iter);
> +	} while (l-- && dev);
> +
> +	sfile->private = iter;
> +	return dev;
>  }
>  
> +static void *scsi_seq_next(struct seq_file *sfile, void *v, loff_t *pos)
> +{
> +	struct klist_iter *iter = (struct klist_iter *)sfile->private;
> +	++*pos;
> +	return next_device(iter);
> +}
> +
> +static void scsi_seq_stop(struct seq_file *sfile, void *v)
> +{
> +	struct klist_iter *iter = (struct klist_iter *)sfile->private;
> +	sfile->private = NULL;
> +	klist_iter_exit(iter);
> +	kfree(iter);
> +}
> +
> +static int scsi_seq_show(struct seq_file *sfile, void *v)
> +{
> +	struct klist_iter *iter = (struct klist_iter *)sfile->private;
> +	struct device *dev = (struct device *)v;
> +	struct klist_node *head;
> +
> +	spin_lock(&iter->i_klist->k_lock);
> +	head = container_of(iter->i_klist->k_list.next,
> +	                    struct klist_node, n_node);
> +	if (&dev->knode_bus == head)
> +		seq_puts(sfile, "Attached devices:\n");
> +	spin_unlock(&iter->i_klist->k_lock);
> +
> +	return proc_print_scsidevice(dev, sfile);
> +}
> +
> +static struct seq_operations scsi_seq_ops = {
> +	.start	= scsi_seq_start,
> +	.next	= scsi_seq_next,
> +	.stop	= scsi_seq_stop,
> +	.show	= scsi_seq_show
> +};
> +
>  static int proc_scsi_open(struct inode *inode, struct file *file)
>  {
>  	/*
>  	 * We don't really needs this for the write case but it doesn't
>  	 * harm either.
>  	 */
> -	return single_open(file, proc_scsi_show, NULL);
> +	return seq_open(file, &scsi_seq_ops);
>  }
>  
>  static struct file_operations proc_scsi_operations = {
> @@ -315,7 +372,7 @@ static struct file_operations proc_scsi_
>  	.read		= seq_read,
>  	.write		= proc_scsi_write,
>  	.llseek		= seq_lseek,
> -	.release	= single_release,
> +	.release	= seq_release,
>  };
>  
>  int __init scsi_init_procfs(void)
> 
> 


- --
Jeff Mahoney
SUSE Labs
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.5 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

iD8DBQFGHqweLPWxlyuTD7IRAvY1AJ9JoRxUzRkH9NoMUZZpaxNXuJQq5wCgo+sf
27SGSl6se9mg6BCCbVz8vXg=
=iCgO
-----END PGP SIGNATURE-----

Re: [PATCH] scsi: iterate over devices individually for /proc/scsi/scsi

[Christoph Hellwig]

On Thu, Apr 12, 2007 at 03:34:32PM -0400, Jeff Mahoney wrote:
>  On systems with very large numbers (> 1600 or so) of SCSI devices,
>  cat /proc/scsi/scsi ends up failing with -ENOMEM. This is due to
>  the show routine simply iterating over all of the devices with
>  bus_for_each_dev(), and trying to dump all of them into the buffer
>  at the same time. On my test system (using scsi_debug with 4064 devices),
>  the output ends up being ~ 632k, far more than kmalloc will typically allow.
> 
>  This patch uses seq_file directly instead of single_file, and breaks up
>  the operations into the 4 seq_file callbacks. The result is that
>  each show() operation only dumps ~ 180 bytes into the buffer at a time
>  so we don't run out of memory.

The simpler answer is don't use /proc/scsi/scsi, but lsscsi instead.
It even has an option to provide /proc/scsi/scsi-style output.

This already came up the last time someone from SuSE sent a patch
to fix an "issue" like that.  That time at least the patch was
massageable into something sane, which this one isn't.  So please
take the advice this time and stop using /proc/scsi/scsi.

Re: [PATCH] scsi: iterate over devices individually for /proc/scsi/scsi

[Jeff Mahoney]

Christoph Hellwig wrote:
> On Thu, Apr 12, 2007 at 03:34:32PM -0400, Jeff Mahoney wrote:
>>  On systems with very large numbers (> 1600 or so) of SCSI devices,
>>  cat /proc/scsi/scsi ends up failing with -ENOMEM. This is due to
>>  the show routine simply iterating over all of the devices with
>>  bus_for_each_dev(), and trying to dump all of them into the buffer
>>  at the same time. On my test system (using scsi_debug with 4064 devices),
>>  the output ends up being ~ 632k, far more than kmalloc will typically allow.
>>
>>  This patch uses seq_file directly instead of single_file, and breaks up
>>  the operations into the 4 seq_file callbacks. The result is that
>>  each show() operation only dumps ~ 180 bytes into the buffer at a time
>>  so we don't run out of memory.
> 
> The simpler answer is don't use /proc/scsi/scsi, but lsscsi instead.
> It even has an option to provide /proc/scsi/scsi-style output.
> 
> This already came up the last time someone from SuSE sent a patch
> to fix an "issue" like that.  That time at least the patch was
> massageable into something sane, which this one isn't.  So please
> take the advice this time and stop using /proc/scsi/scsi.

This should be something a bit more sane. After writing this up, I'm in
complete agreement with you on how much of an unsalvagable mess the
previous patch was.

 On systems with very large numbers (> 1600 or so) of SCSI devices,
 cat /proc/scsi/scsi ends up failing with -ENOMEM. This is due to
 the show routine simply iterating over all of the devices with
 bus_for_each_dev(), and trying to dump all of them into the buffer
 at the same time. On my test system (using scsi_debug with 4064 devices),
 the output ends up being ~ 632k, far more than kmalloc will typically allow.

 This patch defines its own seq_file opreations to iterate over the scsi
 devices.The result is that each show() operation only dumps ~ 180 bytes
 into the buffer at a time so we don't run out of memory.

 If the "Attached devices" header isn't required, we can dump the
 sfile->private bit completely.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>

---

 drivers/scsi/scsi_proc.c |   58 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 6 deletions(-)

--- a/drivers/scsi/scsi_proc.c	2007-10-29 13:56:28.000000000 -0400
+++ b/drivers/scsi/scsi_proc.c	2007-10-29 15:50:57.000000000 -0400
@@ -294,20 +294,66 @@ static ssize_t proc_scsi_write(struct fi
 	return err;
 }
 
-static int proc_scsi_show(struct seq_file *s, void *p)
+static int always_match(struct device *dev, void *data)
 {
-	seq_printf(s, "Attached devices:\n");
-	bus_for_each_dev(&scsi_bus_type, NULL, s, proc_print_scsidevice);
-	return 0;
+	return 1;
+}
+
+static inline struct device *next_scsi_device(struct device *start)
+{
+	struct device *next = bus_find_device(&scsi_bus_type, start, NULL,
+					      always_match);
+	put_device(start);
+	return next;
+}
+
+static void *scsi_seq_start(struct seq_file *sfile, loff_t *pos)
+{
+	struct device *dev = NULL;
+	loff_t n = *pos;
+
+	while ((dev = next_scsi_device(dev))) {
+		if (!n--)
+			break;
+		sfile->private++;
+	}
+	return dev;
+}
+
+static void *scsi_seq_next(struct seq_file *sfile, void *v, loff_t *pos)
+{
+	(*pos)++;
+	sfile->private++;
+	return next_scsi_device(v);
+}
+
+static void scsi_seq_stop(struct seq_file *sfile, void *v)
+{
+	put_device(v);
+}
+
+static int scsi_seq_show(struct seq_file *sfile, void *dev)
+{
+	if (!sfile->private)
+		seq_puts(sfile, "Attached devices:\n");
+
+	return proc_print_scsidevice(dev, sfile);
 }
 
+static struct seq_operations scsi_seq_ops = {
+	.start	= scsi_seq_start,
+	.next	= scsi_seq_next,
+	.stop	= scsi_seq_stop,
+	.show	= scsi_seq_show
+};
+
 static int proc_scsi_open(struct inode *inode, struct file *file)
 {
 	/*
 	 * We don't really needs this for the write case but it doesn't
 	 * harm either.
 	 */
-	return single_open(file, proc_scsi_show, NULL);
+	return seq_open(file, &scsi_seq_ops);
 }
 
 static struct file_operations proc_scsi_operations = {
@@ -315,7 +361,7 @@ static struct file_operations proc_scsi_
 	.read		= seq_read,
 	.write		= proc_scsi_write,
 	.llseek		= seq_lseek,
-	.release	= single_release,
+	.release	= seq_release,
 };
 
 int __init scsi_init_procfs(void)


-- 
Jeff Mahoney
SUSE Labs

Re: [PATCH] scsi: iterate over devices individually for /proc/scsi/scsi

[Christoph Hellwig]

This patch looks a lot better and is how seq_file should be used.  I'm still
not entirely happy about adding more features to this long-deprecated interface,
but if others agree with this patch you'll get my ACK aswell.

[PATCH] scsi: iterate over devices individually for /proc/scsi/scsi

[Jeff Mahoney]

 On systems with very large numbers (> 1600 or so) of SCSI devices,
 cat /proc/scsi/scsi ends up failing with -ENOMEM. This is due to
 the show routine simply iterating over all of the devices with
 bus_for_each_dev(), and trying to dump all of them into the buffer
 at the same time. On my test system (using scsi_debug with 4064 devices),
 the output ends up being ~ 632k, far more than kmalloc will typically allow.

 This patch defines its own seq_file opreations to iterate over the scsi
 devices.The result is that each show() operation only dumps ~ 180 bytes
 into the buffer at a time so we don't run out of memory.

 If the "Attached devices" header isn't required, we can dump the
 sfile->private bit completely.

Signed-off-by: Jeff Mahoney <jeffm@suse.com>
---

 drivers/scsi/scsi_proc.c |   58 ++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 6 deletions(-)

--- a/drivers/scsi/scsi_proc.c
+++ b/drivers/scsi/scsi_proc.c
@@ -386,13 +386,59 @@ static ssize_t proc_scsi_write(struct fi
  * @s: output goes here
  * @p: not used
  */
-static int proc_scsi_show(struct seq_file *s, void *p)
+static int always_match(struct device *dev, void *data)
 {
-	seq_printf(s, "Attached devices:\n");
-	bus_for_each_dev(&scsi_bus_type, NULL, s, proc_print_scsidevice);
-	return 0;
+	return 1;
 }
 
+static inline struct device *next_scsi_device(struct device *start)
+{
+	struct device *next = bus_find_device(&scsi_bus_type, start, NULL,
+					      always_match);
+	put_device(start);
+	return next;
+}
+
+static void *scsi_seq_start(struct seq_file *sfile, loff_t *pos)
+{
+	struct device *dev = NULL;
+	loff_t n = *pos;
+
+	while ((dev = next_scsi_device(dev))) {
+		if (!n--)
+			break;
+		sfile->private++;
+	}
+	return dev;
+}
+
+static void *scsi_seq_next(struct seq_file *sfile, void *v, loff_t *pos)
+{
+	(*pos)++;
+	sfile->private++;
+	return next_scsi_device(v);
+}
+
+static void scsi_seq_stop(struct seq_file *sfile, void *v)
+{
+	put_device(v);
+}
+
+static int scsi_seq_show(struct seq_file *sfile, void *dev)
+{
+	if (!sfile->private)
+		seq_puts(sfile, "Attached devices:\n");
+
+	return proc_print_scsidevice(dev, sfile);
+}
+
+static struct seq_operations scsi_seq_ops = {
+	.start	= scsi_seq_start,
+	.next	= scsi_seq_next,
+	.stop	= scsi_seq_stop,
+	.show	= scsi_seq_show
+};
+
 /**
  * proc_scsi_open - glue function
  * @inode: not used
@@ -406,7 +452,7 @@ static int proc_scsi_open(struct inode *
 	 * We don't really need this for the write case but it doesn't
 	 * harm either.
 	 */
-	return single_open(file, proc_scsi_show, NULL);
+	return seq_open(file, &scsi_seq_ops);
 }
 
 static const struct file_operations proc_scsi_operations = {
@@ -415,7 +461,7 @@ static const struct file_operations proc
 	.read		= seq_read,
 	.write		= proc_scsi_write,
 	.llseek		= seq_lseek,
-	.release	= single_release,
+	.release	= seq_release,
 };
 
 /**
-- 
Jeff Mahoney
SUSE Labs