[PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[Luben Tuikov]

... at device removal.

Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
---
 drivers/scsi/scsi_lib.c    |   10 +++++++---
 drivers/scsi/scsi_scan.c   |    1 +
 include/scsi/scsi_device.h |    3 +++
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 7b0f9a3..f0de7ca 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_q
 			if(specials_only == SDEV_QUIESCE ||
 					specials_only == SDEV_BLOCK)
 				goto defer;
-			
-			sdev_printk(KERN_ERR, sdev,
-				    "rejecting I/O to device being removed\n");
+
+			if (sdev->num_rej_messages > 0) {
+				sdev->num_rej_messages--;
+				sdev_printk(KERN_ERR, sdev,
+					    "rejecting I/O to device "
+					    "being removed\n");
+			}
 			goto kill;
 		}
 			
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 1a5474b..ad988df 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -223,6 +223,7 @@ static struct scsi_device *scsi_alloc_sd
 	INIT_LIST_HEAD(&sdev->cmd_list);
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	spin_lock_init(&sdev->list_lock);
+	sdev->num_rej_messages = SCSI_DEV_NUM_REJ_MESSAGES;
 
 	sdev->sdev_gendev.parent = get_device(&starget->dev);
 	sdev->sdev_target = starget;
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 895d212..4b0785c 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -140,6 +140,9 @@ struct scsi_device {
 
 	struct execute_work	ew; /* used to get process context on put */
 
+#define SCSI_DEV_NUM_REJ_MESSAGES 1
+	int	num_rej_messages;
+
 	enum scsi_device_state sdev_state;
 	unsigned long		sdev_data[0];
 } __attribute__((aligned(sizeof(unsigned long))));
-- 
1.4.3.3.g6cec

Re: [PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[Jens Axboe]

On Fri, Nov 03 2006, Luben Tuikov wrote:
> ... at device removal.
> 
> Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> ---
>  drivers/scsi/scsi_lib.c    |   10 +++++++---
>  drivers/scsi/scsi_scan.c   |    1 +
>  include/scsi/scsi_device.h |    3 +++
>  3 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 7b0f9a3..f0de7ca 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_q
>  			if(specials_only == SDEV_QUIESCE ||
>  					specials_only == SDEV_BLOCK)
>  				goto defer;
> -			
> -			sdev_printk(KERN_ERR, sdev,
> -				    "rejecting I/O to device being removed\n");
> +
> +			if (sdev->num_rej_messages > 0) {
> +				sdev->num_rej_messages--;
> +				sdev_printk(KERN_ERR, sdev,
> +					    "rejecting I/O to device "
> +					    "being removed\n");
> +			}

How about using some variant of printk_ratelimit() instead?


-- 
Jens Axboe

Re: [PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[Luben Tuikov]

--- Jens Axboe <jens.axboe@oracle.com> wrote:
> On Fri, Nov 03 2006, Luben Tuikov wrote:
> > ... at device removal.
> > 
> > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > ---
> >  drivers/scsi/scsi_lib.c    |   10 +++++++---
> >  drivers/scsi/scsi_scan.c   |    1 +
> >  include/scsi/scsi_device.h |    3 +++
> >  3 files changed, 11 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > index 7b0f9a3..f0de7ca 100644
> > --- a/drivers/scsi/scsi_lib.c
> > +++ b/drivers/scsi/scsi_lib.c
> > @@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_q
> >  			if(specials_only == SDEV_QUIESCE ||
> >  					specials_only == SDEV_BLOCK)
> >  				goto defer;
> > -			
> > -			sdev_printk(KERN_ERR, sdev,
> > -				    "rejecting I/O to device being removed\n");
> > +
> > +			if (sdev->num_rej_messages > 0) {
> > +				sdev->num_rej_messages--;
> > +				sdev_printk(KERN_ERR, sdev,
> > +					    "rejecting I/O to device "
> > +					    "being removed\n");
> > +			}
> 
> How about using some variant of printk_ratelimit() instead?

Jens, I didn't think to use such a heavy-weight as printk_ratelimit()
(grabbing irq spinlocks et al), since the device struct would be
"freed" shortly.  FWIW, one message is more than enough. I'm being
liberal with 5, but would gladly revert it back to the original
of one message.

Feel free to resubmit using printk_ratelimit().

     Luben

Re: [PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[Jens Axboe]

On Sat, Nov 04 2006, Luben Tuikov wrote:
> --- Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Fri, Nov 03 2006, Luben Tuikov wrote:
> > > ... at device removal.
> > > 
> > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > > ---
> > >  drivers/scsi/scsi_lib.c    |   10 +++++++---
> > >  drivers/scsi/scsi_scan.c   |    1 +
> > >  include/scsi/scsi_device.h |    3 +++
> > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > index 7b0f9a3..f0de7ca 100644
> > > --- a/drivers/scsi/scsi_lib.c
> > > +++ b/drivers/scsi/scsi_lib.c
> > > @@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_q
> > >  			if(specials_only == SDEV_QUIESCE ||
> > >  					specials_only == SDEV_BLOCK)
> > >  				goto defer;
> > > -			
> > > -			sdev_printk(KERN_ERR, sdev,
> > > -				    "rejecting I/O to device being removed\n");
> > > +
> > > +			if (sdev->num_rej_messages > 0) {
> > > +				sdev->num_rej_messages--;
> > > +				sdev_printk(KERN_ERR, sdev,
> > > +					    "rejecting I/O to device "
> > > +					    "being removed\n");
> > > +			}
> > 
> > How about using some variant of printk_ratelimit() instead?
> 
> Jens, I didn't think to use such a heavy-weight as printk_ratelimit()
> (grabbing irq spinlocks et al), since the device struct would be
> "freed" shortly.  FWIW, one message is more than enough. I'm being
> liberal with 5, but would gladly revert it back to the original
> of one message.

It's a printk, it doesn't matter how heavy weight it is. Just seems
silly to re-invent some sort of printk limiter, when one already exists.

> Feel free to resubmit using printk_ratelimit().

Sorry no plans to do that, I'll let James decide what he wants to take
or not.

-- 
Jens Axboe

Re: [PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[Luben Tuikov]

--- Jens Axboe <jens.axboe@oracle.com> wrote:
> On Sat, Nov 04 2006, Luben Tuikov wrote:
> > --- Jens Axboe <jens.axboe@oracle.com> wrote:
> > > On Fri, Nov 03 2006, Luben Tuikov wrote:
> > > > ... at device removal.
> > > > 
> > > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > > > ---
> > > >  drivers/scsi/scsi_lib.c    |   10 +++++++---
> > > >  drivers/scsi/scsi_scan.c   |    1 +
> > > >  include/scsi/scsi_device.h |    3 +++
> > > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > > index 7b0f9a3..f0de7ca 100644
> > > > --- a/drivers/scsi/scsi_lib.c
> > > > +++ b/drivers/scsi/scsi_lib.c
> > > > @@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_q
> > > >  			if(specials_only == SDEV_QUIESCE ||
> > > >  					specials_only == SDEV_BLOCK)
> > > >  				goto defer;
> > > > -			
> > > > -			sdev_printk(KERN_ERR, sdev,
> > > > -				    "rejecting I/O to device being removed\n");
> > > > +
> > > > +			if (sdev->num_rej_messages > 0) {
> > > > +				sdev->num_rej_messages--;
> > > > +				sdev_printk(KERN_ERR, sdev,
> > > > +					    "rejecting I/O to device "
> > > > +					    "being removed\n");
> > > > +			}
> > > 
> > > How about using some variant of printk_ratelimit() instead?
> > 
> > Jens, I didn't think to use such a heavy-weight as printk_ratelimit()
> > (grabbing irq spinlocks et al), since the device struct would be
> > "freed" shortly.  FWIW, one message is more than enough. I'm being
> > liberal with 5, but would gladly revert it back to the original
> > of one message.
> 
> It's a printk, it doesn't matter how heavy weight it is. Just seems
> silly to re-invent some sort of printk limiter, when one already exists.

As with anything in life -- beauty is in the eye of the beholder.
Having said that, I'll take a patch using printk_ratelimint(), too.

The bottom line was that I didn't want over 2 million messages
of sorts to be printed within very short period of time and
soft-locking all 4 CPUs.

> > Feel free to resubmit using printk_ratelimit().
> 
> Sorry no plans to do that, I'll let James decide what he wants to take
> or not.

You seem to know what is better for upsream Linux, i.e. using
printk_ratelimit() vs. printk() on a device which will be freed.

Why not submit your own version of the same thing "doing it the right
way"?

I'll gladly take your "correct" version.

I've been carrying this patch for 8 months -- git-branch and git-merge
are a wonderful thing -- what makes you think I've any aspirations for
it to go to upstream?  I was merely sharing the conditions I've seen
with SCSI hot-plug and hot-unplug and was surprised that no one has seen
this before and posted a similar patch, for 8 months.

    Luben

Re: [PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[Jens Axboe]

On Sun, Nov 05 2006, Luben Tuikov wrote:
> --- Jens Axboe <jens.axboe@oracle.com> wrote:
> > On Sat, Nov 04 2006, Luben Tuikov wrote:
> > > --- Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > On Fri, Nov 03 2006, Luben Tuikov wrote:
> > > > > ... at device removal.
> > > > > 
> > > > > Signed-off-by: Luben Tuikov <ltuikov@yahoo.com>
> > > > > ---
> > > > >  drivers/scsi/scsi_lib.c    |   10 +++++++---
> > > > >  drivers/scsi/scsi_scan.c   |    1 +
> > > > >  include/scsi/scsi_device.h |    3 +++
> > > > >  3 files changed, 11 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> > > > > index 7b0f9a3..f0de7ca 100644
> > > > > --- a/drivers/scsi/scsi_lib.c
> > > > > +++ b/drivers/scsi/scsi_lib.c
> > > > > @@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_q
> > > > >  			if(specials_only == SDEV_QUIESCE ||
> > > > >  					specials_only == SDEV_BLOCK)
> > > > >  				goto defer;
> > > > > -			
> > > > > -			sdev_printk(KERN_ERR, sdev,
> > > > > -				    "rejecting I/O to device being removed\n");
> > > > > +
> > > > > +			if (sdev->num_rej_messages > 0) {
> > > > > +				sdev->num_rej_messages--;
> > > > > +				sdev_printk(KERN_ERR, sdev,
> > > > > +					    "rejecting I/O to device "
> > > > > +					    "being removed\n");
> > > > > +			}
> > > > 
> > > > How about using some variant of printk_ratelimit() instead?
> > > 
> > > Jens, I didn't think to use such a heavy-weight as printk_ratelimit()
> > > (grabbing irq spinlocks et al), since the device struct would be
> > > "freed" shortly.  FWIW, one message is more than enough. I'm being
> > > liberal with 5, but would gladly revert it back to the original
> > > of one message.
> > 
> > It's a printk, it doesn't matter how heavy weight it is. Just seems
> > silly to re-invent some sort of printk limiter, when one already exists.
> 
> As with anything in life -- beauty is in the eye of the beholder.
> Having said that, I'll take a patch using printk_ratelimint(), too.
> 
> The bottom line was that I didn't want over 2 million messages
> of sorts to be printed within very short period of time and
> soft-locking all 4 CPUs.
> 
> > > Feel free to resubmit using printk_ratelimit().
> > 
> > Sorry no plans to do that, I'll let James decide what he wants to take
> > or not.
> 
> You seem to know what is better for upsream Linux, i.e. using
> printk_ratelimit() vs. printk() on a device which will be freed.
> 
> Why not submit your own version of the same thing "doing it the right
> way"?

"my" version would be really simple - either just use if
(printk_ratelimit()) or add a scsi_printk_ratelimit() with personalized
settings.

I do agree that we have an issue there, we have the potential to
generate oodles of these offline or reject messages and currently
nothing limiting them.

So, again, I'll ask James' (this time cc'ed) opinion on what he thinks.
Perhaps he agrees that we should go the ratelimit route, or perhaps he
likes your version.  If the former, I'll be happy to code that up
(should be a 1 minute job).

-- 
Jens Axboe

Re: [PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[James Bottomley]

On Mon, 2006-11-06 at 22:00 +0100, Jens Axboe wrote:
> So, again, I'll ask James' (this time cc'ed) opinion on what he thinks.
> Perhaps he agrees that we should go the ratelimit route, or perhaps he
> likes your version.  If the former, I'll be happy to code that up
> (should be a 1 minute job).

I agree we should be using the standard tools rather than inventing our
own, so printk_ratelimit() would be the correct way to do this.

James

Re: [PATCH 1/2] [SCSI] Print only a single message "rejecting I/O to device being removed"

[Jens Axboe]

On Tue, Nov 07 2006, James Bottomley wrote:
> On Mon, 2006-11-06 at 22:00 +0100, Jens Axboe wrote:
> > So, again, I'll ask James' (this time cc'ed) opinion on what he thinks.
> > Perhaps he agrees that we should go the ratelimit route, or perhaps he
> > likes your version.  If the former, I'll be happy to code that up
> > (should be a 1 minute job).
> 
> I agree we should be using the standard tools rather than inventing our
> own, so printk_ratelimit() would be the correct way to do this.

Alright, then it turns into something like this:

-----

[SCSI] Rate limit IO reject messages

Otherwise we can flood the system with messages when a device is
removed.

Signed-off-by: Jens Axboe <jens.axboe@oracle.com>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index d2c02df..a935b9d 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1149,7 +1149,8 @@ static int scsi_prep_fn(struct request_q
 			    specials_only == SDEV_BLOCK)
 				goto defer;
 			
-			sdev_printk(KERN_ERR, sdev,
+			if (printk_ratelimit())
+				sdev_printk(KERN_ERR, sdev,
 				    "rejecting I/O to device being removed\n");
 			goto kill;
 		}


-- 
Jens Axboe