Print only a single message "rejecting I/O to device being removed"
PATCH 1/2 Print only a single message "rejecting I/O to device being removed"
... at device removal.
--- 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(-)
drivers/scsi/scsi_lib.c +7 -3
@@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_qif (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; }
drivers/scsi/scsi_scan.c +1 -0
@@ -223,6 +223,7 @@ static struct scsi_device *scsi_alloc_sdINIT_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;
include/scsi/scsi_device.h +3 -0
@@ -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))));
Signature · 1 line
1.4.3.3.g6cec
Quoted 17 lines · Luben Tuikov
... at device removal.
Signed-off-by Luben Tuikov <[email protected]>--- 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(-)drivers/scsi/scsi_lib.c +0
-0@@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_qif (specials_only ==SDEV_QUIESCE || specials_only ==SDEV_BLOCK ) goto defer;
drivers/scsi/scsi_lib.c static int scsi_prep_fn(struct request_q · line 1311
Quoted 10 lines
drivers/scsi/scsi_lib.c +7
-3- -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?
Signature · 1 line
Jens Axboe
Quoted 22 lines · --- Jens Axboe
Quoted 21 lines · Luben Tuikov
... at device removal.
Signed-off-by Luben Tuikov <[email protected]>--- 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(-)drivers/scsi/scsi_lib.c +1
-3@@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_qif (specials_only ==SDEV_QUIESCE || specials_only ==SDEV_BLOCK ) goto defer; - -sdev_printk (KERN_ERR , sdev, -"rejecting I/O to device being removed\n" ); +
Quoted 8 lines
drivers/scsi/scsi_lib.c static int scsi_prep_fn(struct request_q · line 1311
Quoted 6 lines
drivers/scsi/scsi_lib.c +6
-0+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
Quoted 32 lines · Luben Tuikov
Quoted 22 lines · --- Jens Axboe
Quoted 21 lines · Luben Tuikov
... at device removal.
Signed-off-by Luben Tuikov <[email protected]>--- 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(-)drivers/scsi/scsi_lib.c +1
-3@@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_qif (specials_only ==SDEV_QUIESCE || specials_only ==SDEV_BLOCK ) goto defer; - -sdev_printk (KERN_ERR , sdev, -"rejecting I/O to device being removed\n" ); +Quoted 8 lines
drivers/scsi/scsi_lib.c static int scsi_prep_fn(struct request_q · line 1311
Quoted 6 lines
drivers/scsi/scsi_lib.c +6
-0+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?
Quoted 5 lines
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.
Quoted 1 line
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.
Signature · 1 line
Jens Axboe
Quoted 33 lines · --- Jens Axboe
Quoted 32 lines · Luben Tuikov
Quoted 30 lines · --- Jens Axboe
Quoted 17 lines · Luben Tuikov
... at device removal.
Signed-off-by Luben Tuikov <[email protected]>--- 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(-)drivers/scsi/scsi_lib.c +0
-0@@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_qif (specials_only ==SDEV_QUIESCE || specials_only ==SDEV_BLOCK ) goto defer;drivers/scsi/scsi_lib.c static int scsi_prep_fn(struct request_q · line 1311
Quoted 10 lines
drivers/scsi/scsi_lib.c +7
-3- -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?
Quoted 8 lines
Quoted 5 lines
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.
Quoted 4 lines
Quoted 1 line
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
Quoted 52 lines · Luben Tuikov
Quoted 33 lines · --- Jens Axboe
Quoted 32 lines · Luben Tuikov
Quoted 30 lines · --- Jens Axboe
Quoted 17 lines · Luben Tuikov
... at device removal.
Signed-off-by Luben Tuikov <[email protected]>--- 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(-)drivers/scsi/scsi_lib.c +0
-0@@ -1302,9 +1302,13 @@ static int scsi_prep_fn(struct request_qif (specials_only ==SDEV_QUIESCE || specials_only ==SDEV_BLOCK ) goto defer;drivers/scsi/scsi_lib.c static int scsi_prep_fn(struct request_q · line 1311
Quoted 10 lines
drivers/scsi/scsi_lib.c +7
-3- -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?Quoted 8 lines
Quoted 5 lines
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.
Quoted 2 lines
Quoted 1 line
Feel free to resubmit using
printk_ratelimit().
Quoted 8 lines
Quoted 2 lines
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).
Signature · 1 line
Jens Axboe
Quoted 4 lines · Jens Axboe
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
Quoted 8 lines · James Bottomley
Quoted 4 lines · Jens Axboe
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.
drivers/scsi/scsi_lib.c +2 -1
@@ -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; }
Signature · 1 line
Jens Axboe