Use proper seq_file api for /proc/scsi/scsi
PATCH 1/1 Use proper seq_file api for /proc/scsi/scsi
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.
This patch impelements the proper seq_file interface which prints out all devices sequentially.
The use of 'get_device()/put_device()' is interesting, as it relies on ->show being called after a successful call to ->start / ->next. But the current seq_file implementation does it that way; and using ->stop doesn't quite work as it's end up being called several times for no appearent reason.
But I'm all ears for a better solution.
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke [email protected] SuSE Linux AG S390 & zSeries Maxfeldstraße 5 +49 911 74053 688 90409 Nürnberg http://www.suse.de
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.
drivers/scsi/scsi_proc.c renamed from drivers/scsi/scsi_proc.c.orig +95 -12
@@ -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_Hostremove_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(structelse 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 fireturn 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 )
Quoted 5 lines · Hannes Reinecke
Hi all,
/proc/scsi/scsicurrently has a very dumb implementation of theseq_fileapi which causes 'cat/proc/scsi/scsi' to return with-ENOMEMwhen 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.
Quoted 11 lines · Christoph Hellwig
Quoted 5 lines · Hannes Reinecke
Hi all,
/proc/scsi/scsicurrently has a very dumb implementation of theseq_fileapi which causes 'cat/proc/scsi/scsi' to return with-ENOMEMwhen a large amount of devices are connected.
/proc/scsi/scsiis 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
Signature · 8 lines
Dr. Hannes Reinecke [email protected] SuSE Linux AG S390 & zSeries Maxfeldstraße 5 +49 911 74053 688 90409 Nürnberg http://www.suse.de - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 6 lines · Hannes Reinecke
Quoted 4 lines
/proc/scsi/scsiis 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.
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Hannes Reinecke
Quoted 4 lines
/proc/scsi/scsiis 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
Quoted 6 lines · Jeremy Higdon
Quoted 2 lines
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.
Quoted 12 lines · Christoph Hellwig
Quoted 6 lines · Jeremy Higdon
Quoted 2 lines
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
Quoted 5 lines
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...
Quoted 6 lines · Christoph Hellwig
Quoted 3 lines · Jeremy Higdon
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.
Quoted 8 lines
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
Quoted 2 lines · Jeremy Higdon
Just as a sanity check, you meant "lsscsi" and not "lssci" in your original reply, right?
Yes.