| Message ID | 1617325783-20740-1-git-send-email-loberman@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | scsi_mod: Add a new parameter to scsi_mod to control storage logging | expand |
On 4/1/21 6:09 PM, Laurence Oberman wrote: > #define sd_printk(prefix, sdsk, fmt, a...) \ > - (sdsk)->disk ? \ > - sdev_prefix_printk(prefix, (sdsk)->device, \ > + do { \ > + if (!storage_quiet_discovery) \ > + (sdsk)->disk ? \ > + sdev_prefix_printk(prefix, (sdsk)->device, \ > (sdsk)->disk->disk_name, fmt, ##a) : \ > - sdev_printk(prefix, (sdsk)->device, fmt, ##a) > + sdev_printk(prefix, (sdsk)->device, fmt, ##a); \ > + } while (0) The indentation inside the above macro looks odd to me. I guess that you want to avoid deep indentation? Consider using if (...) break; instead to reduce the indentation level. Additionally, please change the ternary operator into an if-condition since the result of the ternary operator is not used. How about rewriting the above macro as follows? do { if (storage_quiet_discovery) break; if ((sdk)->disk) [ ... ] else [ ... ] } while (0) Thanks, Bart.
On Thu, 2021-04-01 at 18:33 -0700, Bart Van Assche wrote: > On 4/1/21 6:09 PM, Laurence Oberman wrote: > > #define sd_printk(prefix, sdsk, fmt, a...) > > \ > > - (sdsk)->disk ? > > \ > > - sdev_prefix_printk(prefix, (sdsk)->device, \ > > + do { > > \ > > + if (!storage_quiet_discovery) > > \ > > + (sdsk)->disk ? > > \ > > + sdev_prefix_printk(prefix, (sdsk)->device, \ > > (sdsk)->disk->disk_name, fmt, ##a) : > > \ > > - sdev_printk(prefix, (sdsk)->device, fmt, ##a) > > + sdev_printk(prefix, (sdsk)->device, fmt, ##a); > > \ > > + } while (0) > > The indentation inside the above macro looks odd to me. I guess that > you > want to avoid deep indentation? Consider using if (...) break; > instead > to reduce the indentation level. Additionally, please change the > ternary > operator into an if-condition since the result of the ternary > operator > is not used. How about rewriting the above macro as follows? > > do { > if (storage_quiet_discovery) > break; > if ((sdk)->disk) > [ ... ] > else > [ ... ] > } while (0) > > Thanks, > > Bart. > Hi Bart Yes, Thanks I can try that option for the macro and clean it up. I will wait a bit for others to review so I can attend to all changes at once. Note that the original version was indented like that too and has the ternary. See here: #define sd_printk(prefix, sdsk, fmt, a...) \ (sdsk)->disk ? \ sdev_prefix_printk(prefix, (sdsk)- >device, \ (sdsk)->disk->disk_name, fmt, ##a) : \ sdev_printk(prefix, (sdsk)->device, fmt, ##a)
On Fri, 2021-04-02 at 08:37 -0400, Laurence Oberman wrote: > On Thu, 2021-04-01 at 18:33 -0700, Bart Van Assche wrote: > > On 4/1/21 6:09 PM, Laurence Oberman wrote: > > > #define sd_printk(prefix, sdsk, fmt, a...) > > > > > > \ > > > - (sdsk)->disk ? > > > \ > > > - sdev_prefix_printk(prefix, (sdsk)->device, \ > > > + do { > > > \ > > > + if (!storage_quiet_discovery) > > > \ > > > + (sdsk)->disk ? > > > \ > > > + sdev_prefix_printk(prefix, (sdsk)->device, \ > > > (sdsk)->disk->disk_name, fmt, ##a) : > > > \ > > > - sdev_printk(prefix, (sdsk)->device, fmt, ##a) > > > + sdev_printk(prefix, (sdsk)->device, fmt, ##a); > > > \ > > > + } while (0) > > > > The indentation inside the above macro looks odd to me. I guess > > that > > you > > want to avoid deep indentation? Consider using if (...) break; > > instead > > to reduce the indentation level. Additionally, please change the > > ternary > > operator into an if-condition since the result of the ternary > > operator > > is not used. How about rewriting the above macro as follows? > > > > do { > > if (storage_quiet_discovery) > > break; > > if ((sdk)->disk) > > [ ... ] > > else > > [ ... ] > > } while (0) > > > > Thanks, > > > > Bart. > > > > Hi Bart > Yes, Thanks I can try that option for the macro and clean it up. > I will wait a bit for others to review so I can attend to all changes > at once. > > Note that the original version was indented like that too and has the > ternary. > > See here: > #define sd_printk(prefix, sdsk, fmt, > a...) \ > (sdsk)->disk > ? \ > sdev_prefix_printk(prefix, (sdsk)- > > device, \ > > (sdsk)->disk->disk_name, fmt, ##a) > : \ > sdev_printk(prefix, (sdsk)->device, fmt, ##a) > Hi Bart the original macro is the same so I think best not to change it other than the wrapper I added. What are your thoughts. Gentle ping for any other reviews: We are actively using this at some customers so it would be good to know if others upstream will NAK this or have any comments. Sincerely Laurence Oberman
On Thu, 2021-04-01 at 21:09 -0400, Laurence Oberman wrote: > This new parameter storage_quiet_discovery defaults to 0 and behavior is > unchanged. If its set to 1 on the kernel line then sd_printk and > sdev_printk are disabled for printing. The default logging can > be re-enabled any time after boot using /etc/sysctl.conf by > setting dev.scsi.storage_quiet_discovery = 0. > systctl -w dev.scsi.storage_quiet_discovery=0 will also change it > immediately back to logging. > Users can leave it set to 1 on the kernel line and 0 in the conf > file so it changes back to default after rc.sysinit. > This solves the tough problem of systems with 1000's of storage LUNS > consuming a system and preventing it from booting due to NMI's and > timeouts. > > Signed-off-by: Laurence Oberman <loberman@redhat.com> > --- > Documentation/scsi/scsi-parameters.rst | 16 ++++++++++++++++ > drivers/scsi/scsi.c | 6 ++++++ > drivers/scsi/scsi_sysctl.c | 5 +++++ > drivers/scsi/sd.h | 11 ++++++++--- > include/scsi/scsi_device.h | 8 ++++++-- > 5 files changed, 41 insertions(+), 5 deletions(-) > > diff --git a/Documentation/scsi/scsi-parameters.rst b/Documentation/scsi/scsi-parameters.rst > index c42c55e1e25e..3c6284ef7fd5 100644 > --- a/Documentation/scsi/scsi-parameters.rst > +++ b/Documentation/scsi/scsi-parameters.rst > @@ -93,6 +93,22 @@ parameters may be changed at runtime by the command > S390-tools package, available for download at > https://github.com/ibm-s390-linux/s390-tools/blob/master/scripts/scsi_logging_level > > + scsi_mod.storage_quiet_discovery= > + [SCSI] a parameter to control the printing from > + sdev_printk and sd_printk for systems with large > + amounts of LUNS. > + Defaults to 0 so unchanged behavior. > + If scsi_mod.storage_quiet_discovery=1 is added boot line > + then the messages are not printed and can be enabled > + after boot via > + echo 0 > > + /sys/module/scsi_mod/parameters/storage_quiet_discovery > + Another option is using > + sysctl -w dev.scsi.storage_quiet_discovery=0. > + Leaving this set to 0 in /etc/sysctl.conf and setting > + it to 1 on the kernel line will help for these large > + LUN count configurations and ensure its back on after boot. > + > scsi_mod.scan= [SCSI] sync (default) scans SCSI busses as they are > discovered. async scans them in kernel threads, > allowing boot to proceed. none ignores them, expecting > diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c > index 24619c3bebd5..b6b1b9ecc30e 100644 > --- a/drivers/scsi/scsi.c > +++ b/drivers/scsi/scsi.c > @@ -86,6 +86,9 @@ unsigned int scsi_logging_level; > EXPORT_SYMBOL(scsi_logging_level); > #endif > > +int storage_quiet_discovery = 0; > +EXPORT_SYMBOL(storage_quiet_discovery); > + > /* > * Domain for asynchronous system resume operations. It is marked 'exclusive' > * to avoid being included in the async_synchronize_full() that is invoked by > @@ -749,6 +752,9 @@ MODULE_LICENSE("GPL"); > > module_param(scsi_logging_level, int, S_IRUGO|S_IWUSR); > MODULE_PARM_DESC(scsi_logging_level, "a bit mask of logging levels"); > +module_param(storage_quiet_discovery, int, S_IRUGO|S_IWUSR); > +MODULE_PARM_DESC(storage_quiet_discovery, "If set to 1 will silence SCSI and \ > + ALUA discovery logging on boot"); > > static int __init init_scsi(void) > { > diff --git a/drivers/scsi/scsi_sysctl.c b/drivers/scsi/scsi_sysctl.c > index 7259704a7f52..db9e4520ad4e 100644 > --- a/drivers/scsi/scsi_sysctl.c > +++ b/drivers/scsi/scsi_sysctl.c > @@ -18,6 +18,11 @@ static struct ctl_table scsi_table[] = { > .maxlen = sizeof(scsi_logging_level), > .mode = 0644, > .proc_handler = proc_dointvec }, > + { .procname = "storage_quiet_discovery", > + .data = &storage_quiet_discovery, > + .maxlen = sizeof(storage_quiet_discovery), > + .mode = 0644, > + .proc_handler = proc_dointvec }, > { } > }; > > diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h > index b59136c4125b..ede4d53a232a 100644 > --- a/drivers/scsi/sd.h > +++ b/drivers/scsi/sd.h > @@ -133,11 +133,16 @@ static inline struct scsi_disk *scsi_disk(struct gendisk *disk) > return container_of(disk->private_data, struct scsi_disk, driver); > } > > +extern int storage_quiet_discovery; > + > #define sd_printk(prefix, sdsk, fmt, a...) \ > - (sdsk)->disk ? \ > - sdev_prefix_printk(prefix, (sdsk)->device, \ > + do { \ > + if (!storage_quiet_discovery) \ > + (sdsk)->disk ? \ > + sdev_prefix_printk(prefix, (sdsk)->device, \ > (sdsk)->disk->disk_name, fmt, ##a) : \ > - sdev_printk(prefix, (sdsk)->device, fmt, ##a) > + sdev_printk(prefix, (sdsk)->device, fmt, ##a); \ > + } while (0) > > #define sd_first_printk(prefix, sdsk, fmt, a...) \ > do { \ > diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h > index 1a5c9a3df6d6..ca71aa784d1e 100644 > --- a/include/scsi/scsi_device.h > +++ b/include/scsi/scsi_device.h > @@ -258,8 +258,12 @@ __printf(4, 5) void > sdev_prefix_printk(const char *, const struct scsi_device *, const char *, > const char *, ...); > > -#define sdev_printk(l, sdev, fmt, a...) \ > - sdev_prefix_printk(l, sdev, NULL, fmt, ##a) > +extern int storage_quiet_discovery; > + > +#define sdev_printk(l, sdev, fmt, a...) ({ \ > + if (!storage_quiet_discovery) \ > + sdev_prefix_printk(l, sdev, NULL, fmt, ##a); \ > + }) > > __printf(3, 4) void > scmd_printk(const char *, const struct scsi_cmnd *, const char *, ...); Reviewed-by: Ewan D. Milne <emilne@redhat.com>
On 4/8/21 8:56 AM, Laurence Oberman wrote: > Hi Bart the original macro is the same so I think best not to change it > other than the wrapper I added. What are your thoughts. Hi Laurence, Since the current definition is not optimal from a source code readability standpoint and since this patch changes the sd_printk() definition I think this is a great opportunity to improve that definition. Please note that I don't have a strong opinion about this. Bart.
Good morning Laurence. I guess we still need Martin's opinion, but considering how many customers I've had to help get past this, I'd welcome an easy way out. Thanks. Reviewed-by: John Pittman <jpittman@redhat.com> On Fri, Apr 9, 2021 at 4:59 PM Bart Van Assche <bvanassche@acm.org> wrote: > > On 4/8/21 8:56 AM, Laurence Oberman wrote: > > Hi Bart the original macro is the same so I think best not to change it > > other than the wrapper I added. What are your thoughts. > > Hi Laurence, > > Since the current definition is not optimal from a source code > readability standpoint and since this patch changes the sd_printk() > definition I think this is a great opportunity to improve that > definition. Please note that I don't have a strong opinion about this. > > Bart. >
diff --git a/Documentation/scsi/scsi-parameters.rst b/Documentation/scsi/scsi-parameters.rst index c42c55e1e25e..3c6284ef7fd5 100644 --- a/Documentation/scsi/scsi-parameters.rst +++ b/Documentation/scsi/scsi-parameters.rst @@ -93,6 +93,22 @@ parameters may be changed at runtime by the command S390-tools package, available for download at https://github.com/ibm-s390-linux/s390-tools/blob/master/scripts/scsi_logging_level + scsi_mod.storage_quiet_discovery= + [SCSI] a parameter to control the printing from + sdev_printk and sd_printk for systems with large + amounts of LUNS. + Defaults to 0 so unchanged behavior. + If scsi_mod.storage_quiet_discovery=1 is added boot line + then the messages are not printed and can be enabled + after boot via + echo 0 > + /sys/module/scsi_mod/parameters/storage_quiet_discovery + Another option is using + sysctl -w dev.scsi.storage_quiet_discovery=0. + Leaving this set to 0 in /etc/sysctl.conf and setting + it to 1 on the kernel line will help for these large + LUN count configurations and ensure its back on after boot. + scsi_mod.scan= [SCSI] sync (default) scans SCSI busses as they are discovered. async scans them in kernel threads, allowing boot to proceed. none ignores them, expecting diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c index 24619c3bebd5..b6b1b9ecc30e 100644 --- a/drivers/scsi/scsi.c +++ b/drivers/scsi/scsi.c @@ -86,6 +86,9 @@ unsigned int scsi_logging_level; EXPORT_SYMBOL(scsi_logging_level); #endif +int storage_quiet_discovery = 0; +EXPORT_SYMBOL(storage_quiet_discovery); + /* * Domain for asynchronous system resume operations. It is marked 'exclusive' * to avoid being included in the async_synchronize_full() that is invoked by @@ -749,6 +752,9 @@ MODULE_LICENSE("GPL"); module_param(scsi_logging_level, int, S_IRUGO|S_IWUSR); MODULE_PARM_DESC(scsi_logging_level, "a bit mask of logging levels"); +module_param(storage_quiet_discovery, int, S_IRUGO|S_IWUSR); +MODULE_PARM_DESC(storage_quiet_discovery, "If set to 1 will silence SCSI and \ + ALUA discovery logging on boot"); static int __init init_scsi(void) { diff --git a/drivers/scsi/scsi_sysctl.c b/drivers/scsi/scsi_sysctl.c index 7259704a7f52..db9e4520ad4e 100644 --- a/drivers/scsi/scsi_sysctl.c +++ b/drivers/scsi/scsi_sysctl.c @@ -18,6 +18,11 @@ static struct ctl_table scsi_table[] = { .maxlen = sizeof(scsi_logging_level), .mode = 0644, .proc_handler = proc_dointvec }, + { .procname = "storage_quiet_discovery", + .data = &storage_quiet_discovery, + .maxlen = sizeof(storage_quiet_discovery), + .mode = 0644, + .proc_handler = proc_dointvec }, { } }; diff --git a/drivers/scsi/sd.h b/drivers/scsi/sd.h index b59136c4125b..ede4d53a232a 100644 --- a/drivers/scsi/sd.h +++ b/drivers/scsi/sd.h @@ -133,11 +133,16 @@ static inline struct scsi_disk *scsi_disk(struct gendisk *disk) return container_of(disk->private_data, struct scsi_disk, driver); } +extern int storage_quiet_discovery; + #define sd_printk(prefix, sdsk, fmt, a...) \ - (sdsk)->disk ? \ - sdev_prefix_printk(prefix, (sdsk)->device, \ + do { \ + if (!storage_quiet_discovery) \ + (sdsk)->disk ? \ + sdev_prefix_printk(prefix, (sdsk)->device, \ (sdsk)->disk->disk_name, fmt, ##a) : \ - sdev_printk(prefix, (sdsk)->device, fmt, ##a) + sdev_printk(prefix, (sdsk)->device, fmt, ##a); \ + } while (0) #define sd_first_printk(prefix, sdsk, fmt, a...) \ do { \ diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h index 1a5c9a3df6d6..ca71aa784d1e 100644 --- a/include/scsi/scsi_device.h +++ b/include/scsi/scsi_device.h @@ -258,8 +258,12 @@ __printf(4, 5) void sdev_prefix_printk(const char *, const struct scsi_device *, const char *, const char *, ...); -#define sdev_printk(l, sdev, fmt, a...) \ - sdev_prefix_printk(l, sdev, NULL, fmt, ##a) +extern int storage_quiet_discovery; + +#define sdev_printk(l, sdev, fmt, a...) ({ \ + if (!storage_quiet_discovery) \ + sdev_prefix_printk(l, sdev, NULL, fmt, ##a); \ + }) __printf(3, 4) void scmd_printk(const char *, const struct scsi_cmnd *, const char *, ...);
This new parameter storage_quiet_discovery defaults to 0 and behavior is unchanged. If its set to 1 on the kernel line then sd_printk and sdev_printk are disabled for printing. The default logging can be re-enabled any time after boot using /etc/sysctl.conf by setting dev.scsi.storage_quiet_discovery = 0. systctl -w dev.scsi.storage_quiet_discovery=0 will also change it immediately back to logging. Users can leave it set to 1 on the kernel line and 0 in the conf file so it changes back to default after rc.sysinit. This solves the tough problem of systems with 1000's of storage LUNS consuming a system and preventing it from booting due to NMI's and timeouts. Signed-off-by: Laurence Oberman <loberman@redhat.com> --- Documentation/scsi/scsi-parameters.rst | 16 ++++++++++++++++ drivers/scsi/scsi.c | 6 ++++++ drivers/scsi/scsi_sysctl.c | 5 +++++ drivers/scsi/sd.h | 11 ++++++++--- include/scsi/scsi_device.h | 8 ++++++-- 5 files changed, 41 insertions(+), 5 deletions(-)