From: Hannes Reinecke <[email protected]>
Subject: cat /proc/scsi/scsi crashes with 'out of memory'
Reference: 75899
'cat /proc/scsi/scsi' returns 'out of memory' when a large number of devices
is connected. This is due to the use of the simple interface into seq_file
which tries to allocate a buffer holding _all_ devices.
This patch converts the proc interface to use the seq_file API properly.
Signed-off-by: Hannes Reinecke <[email protected]>
--- linux-2.6.5/drivers/scsi/scsi_proc.c.orig 2004-04-04 05:36:17.000000000 +0200
+++ linux-2.6.5/drivers/scsi/scsi_proc.c 2005-04-07 11:08:16.877718912 +0200
@@ -25,6 +25,7 @@
#include <linux/errno.h>
#include <linux/blkdev.h>
#include <linux/seq_file.h>
+#include <linux/list.h>
#include <asm/uaccess.h>
#include <scsi/scsi_host.h>
@@ -141,10 +142,90 @@ void scsi_proc_host_rm(struct Scsi_Host
remove_proc_entry(name, shost->hostt->proc_dir);
}
-static int proc_print_scsidevice(struct device *dev, void *data)
+/*
+ * Simple selector for the next device in list.
+ * We take a reference on the current device to
+ * avoid it having vanished underneath us.
+ */
+static int proc_print_sdev_select( struct device *d, void *p)
+{
+ struct device **t = p;
+ struct list_head *l;
+
+ if (!*t) {
+ get_device(d);
+ *t = d;
+ return 1;
+ }
+
+ l = &((*t)->bus_list);
+ if (list_entry(l->next, struct device, bus_list) == d ) {
+ get_device(d);
+ *t = d;
+ return 1;
+ }
+
+ return 0;
+}
+
+/*
+ * seq_file interface
+ * The iterator is of type 'struct device *' and points to the
+ * current device. A reference to the current device is taken
+ * by the selector function above and must be dropped during
+ * the show function.
+ */
+static void *proc_print_sdev_start(struct seq_file *s, loff_t *pos)
+{
+ struct device *d = NULL;
+
+ if (*pos != 0)
+ return NULL;
+
+ seq_printf(s, "Attached devices:\n");
+
+ if ( bus_for_each_dev(&scsi_bus_type, NULL, &d,
+ proc_print_sdev_select) > 0 )
+ return d;
+
+ return NULL;
+}
+
+/*
+ * fetch next device.
+ * We don't actually need the pos argument but seq_file is unhappy
+ * without it.
+ */
+static void *proc_print_sdev_next(struct seq_file *s, void *p, loff_t *pos)
+{
+ struct device *d = p;
+
+ (*pos)++;
+
+ if ( bus_for_each_dev(&scsi_bus_type, p, &d,
+ proc_print_sdev_select) > 0 )
+ return d;
+
+ return NULL;
+}
+
+/*
+ * Stop device iteration.
+ * Nothing to be done here.
+ */
+static void proc_print_sdev_stop(struct seq_file *s, void *p)
+{
+ return;
+}
+
+/*
+ * Show selected device.
+ * We have to dereference the device here.
+ */
+static int proc_print_sdev_show(struct seq_file *s, void *p)
{
- struct scsi_device *sdev = to_scsi_device(dev);
- struct seq_file *s = data;
+ struct device *d = p;
+ struct scsi_device *sdev = to_scsi_device(d);
int i;
seq_printf(s,
@@ -186,9 +267,18 @@ static int proc_print_scsidevice(struct
else
seq_printf(s, "\n");
+ put_device(d);
+
return 0;
}
+struct seq_operations scsi_proc_print_op = {
+ .start = proc_print_sdev_start,
+ .next = proc_print_sdev_next,
+ .stop = proc_print_sdev_stop,
+ .show = proc_print_sdev_show
+};
+
static int scsi_add_single_device(uint host, uint channel, uint id, uint lun)
{
struct Scsi_Host *shost;
@@ -283,20 +373,13 @@ static ssize_t proc_scsi_write(struct fi
return err;
}
-static int proc_scsi_show(struct seq_file *s, void *p)
-{
- seq_printf(s, "Attached devices:\n");
- bus_for_each_dev(&scsi_bus_type, NULL, s, proc_print_scsidevice);
- return 0;
-}
-
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_proc_print_op);
}
static struct file_operations proc_scsi_operations = {
@@ -304,7 +387,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)
On Thu, Apr 07, 2005 at 11:46:27AM +0200, Hannes Reinecke wrote:
> Hi all,
>
> /proc/scsi/scsi currently has a very dumb implementation of the seq_file
> api which causes 'cat /proc/scsi/scsi' to return with -ENOMEM when a
> large amount of devices are connected.
/proc/scsi/scsi is deprecated and even only compiled in if
"legacy /proc/scsi/ support" is enabled. Please move over to lssci which
is using sysfs ASAP.
Christoph Hellwig wrote:
> On Thu, Apr 07, 2005 at 11:46:27AM +0200, Hannes Reinecke wrote:
>>Hi all,
>>
>>/proc/scsi/scsi currently has a very dumb implementation of the seq_file
>>api which causes 'cat /proc/scsi/scsi' to return with -ENOMEM when a
>>large amount of devices are connected.
>
> /proc/scsi/scsi is deprecated and even only compiled in if
> "legacy /proc/scsi/ support" is enabled. Please move over to lssci which
> is using sysfs ASAP.
>
Ah. And that's enough reason for it not to work properly?
Deprecated as it may be, but one could at least expect it to _work_.
Puzzled.
Cheers,
Hannes
--
Dr. Hannes Reinecke [email protected]
SuSE Linux AG S390 & zSeries
Maxfeldstra?e 5 +49 911 74053 688
90409 N?rnberg http://www.suse.de
On Thu, Apr 07, 2005 at 01:21:23PM +0200, Hannes Reinecke wrote:
> > /proc/scsi/scsi is deprecated and even only compiled in if
> > "legacy /proc/scsi/ support" is enabled. Please move over to lssci which
> > is using sysfs ASAP.
> >
> Ah. And that's enough reason for it not to work properly?
> Deprecated as it may be, but one could at least expect it to _work_.
It works for those setups that already worked with 2.4.x, aka only a few
luns.
On Thu, Apr 07, 2005 at 12:24:12PM +0100, Christoph Hellwig wrote:
> On Thu, Apr 07, 2005 at 01:21:23PM +0200, Hannes Reinecke wrote:
> > > /proc/scsi/scsi is deprecated and even only compiled in if
> > > "legacy /proc/scsi/ support" is enabled. Please move over to lssci which
> > > is using sysfs ASAP.
> > >
> > Ah. And that's enough reason for it not to work properly?
> > Deprecated as it may be, but one could at least expect it to _work_.
>
> It works for those setups that already worked with 2.4.x, aka only a few
> luns.
Even if it's deprecated, wouldn't it be good to fix it as long as
it's there, unless it hurts something else? Or at least fix the
out of memory error, even if it doesn't display all the luns?
jeremy
On Fri, 2005-04-08 at 08:56 +0100, Christoph Hellwig wrote:
> On Fri, Apr 08, 2005 at 12:23:46AM -0700, Jeremy Higdon wrote:
> > > It works for those setups that already worked with 2.4.x, aka only a few
> > > luns.
> >
> > Even if it's deprecated, wouldn't it be good to fix it as long as
> > it's there, unless it hurts something else? Or at least fix the
> > out of memory error, even if it doesn't display all the luns?
>
> What other error would you return? I don't particularly care what exact
> error code to return, but putting in Hannes patch would be a bad idea because
> it
>
> a) poke deep into driver model internals, and we absolutely want to avoid
> that
> b) sets a bad precedence that we'll continue adding features to deprecated
> interface and thus encurage people to contiue using it. Note that
> /proc/scsi/* has been deprecated since mid-2.5.x.
so how about starting to remove it?
eg give it its final resting date, start defaulting the config option to
"n" and such...
On Fri, Apr 08, 2005 at 12:23:46AM -0700, Jeremy Higdon wrote:
> > It works for those setups that already worked with 2.4.x, aka only a few
> > luns.
>
> Even if it's deprecated, wouldn't it be good to fix it as long as
> it's there, unless it hurts something else? Or at least fix the
> out of memory error, even if it doesn't display all the luns?
What other error would you return? I don't particularly care what exact
error code to return, but putting in Hannes patch would be a bad idea because
it
a) poke deep into driver model internals, and we absolutely want to avoid
that
b) sets a bad precedence that we'll continue adding features to deprecated
interface and thus encurage people to contiue using it. Note that
/proc/scsi/* has been deprecated since mid-2.5.x.
On Fri, Apr 08, 2005 at 08:56:43AM +0100, Christoph Hellwig wrote:
> On Fri, Apr 08, 2005 at 12:23:46AM -0700, Jeremy Higdon wrote:
> > Even if it's deprecated, wouldn't it be good to fix it as long as
> > it's there, unless it hurts something else? Or at least fix the
> > out of memory error, even if it doesn't display all the luns?
>
> What other error would you return? I don't particularly care what exact
Hmm. Well maybe you're right -- an error is probably better than no
error if not printing all devices.
> error code to return, but putting in Hannes patch would be a bad idea because
> it
>
> a) poke deep into driver model internals, and we absolutely want to avoid
> that
> b) sets a bad precedence that we'll continue adding features to deprecated
> interface and thus encurage people to contiue using it. Note that
> /proc/scsi/* has been deprecated since mid-2.5.x.
Just as a sanity check, you meant "lsscsi" and not "lssci" in your original
reply, right?
jeremy
On Fri, Apr 08, 2005 at 01:10:11AM -0700, Jeremy Higdon wrote:
> Just as a sanity check, you meant "lsscsi" and not "lssci" in your original
> reply, right?
Yes.