[PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[Laurence Oberman]

    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 *, ...);
-- 
2.27.0

Re: [PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[Bart Van Assche]

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.

Re: [PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[Laurence Oberman]

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)

Re: [PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[Laurence Oberman]

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

Re: [PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[Ewan D. Milne]

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>

Re: [PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[Bart Van Assche]

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.

Re: [PATCH] scsi_mod: Add a new parameter to scsi_mod to control storage logging

[John Pittman]

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.
>