Re: [PATCH v6 7/7] scsi_io_completion convert BUGs to WARNs
On Tue, Jun 26, 2018 at 12:53:32PM +0200, Douglas Gilbert wrote: > The SCSI subsystem may not be the primary storage medium on a system, so > bringing down the system because the SCSI mid-level has an unrecoverable > error may be overkill. Think an embedded system with the rootfs on a > /dev/mmcblkp device with the only "SCSI" device being an external > USB port for a memory key (I was fighting with a 3D printer was just that > config a few days ago). Then the user plugs in the world's worst USB > key (billion of candidates). Isn't one error strategy just to report, > ignore and continue? Exactly and even if SCSI is the primary storage medium BUG_ON()s are just bad, sorry. It's always better to do error recovery then hard crashing a machine as it may be able to write out logs when doing error recovery so one actually has a chance debugging the reasons why it happened. Byte, Johannes -- Johannes Thumshirn Storage jthumsh...@suse.de+49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
Re: [PATCH v6 7/7] scsi_io_completion convert BUGs to WARNs
On 2018-06-26 07:28 AM, Hannes Reinecke wrote:
On 06/23/2018 12:22 PM, Douglas Gilbert wrote:
The scsi_io_completion function contains three BUG() and BUG_ON() calls.
Replace them with WARN variants.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
Reviewed-by: Bart Van Assche
---
drivers/scsi/scsi_lib.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 19ed11abe886..252edd61a688 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1060,13 +1060,21 @@ void scsi_io_completion(struct scsi_cmnd *cmd,
unsigned int good_bytes)
scsi_req(req->next_rq)->resid_len = scsi_in(cmd)->resid;
if (scsi_end_request(req, BLK_STS_OK, blk_rq_bytes(req),
blk_rq_bytes(req->next_rq)))
- BUG();
+ WARN_ONCE(true,
+ "Bidi command with remaining bytes");
return;
}
}
/* no bidi support yet, other than in pass-through */
- BUG_ON(blk_bidi_rq(req));
+ if (unlikely(blk_bidi_rq(req))) {
+ WARN_ONCE(true, "Only support bidi command in passthrough");
+ scmd_printk(KERN_ERR, cmd, "Killing bidi command\n");
+ if (scsi_end_request(req, BLK_STS_IOERR, blk_rq_bytes(req),
+ blk_rq_bytes(req->next_rq)))
+ WARN_ONCE(true, "Bidi command with remaining bytes");
+ return;
+ }
/*
* Next deal with any sectors which we were able to correctly
@@ -1089,7 +1097,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
/* Kill remainder if no retries. */
if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
- BUG();
+ WARN_ONCE(true,
+ "Bytes remaining after failed, no-retry command");
return;
}
So what happens to these requests if not all bytes could be finished?
Do we have an error recovery strategy?
Changing to WARN_ON would only makes sense if we have a way of recovering the
failed request, otherwise we'd have to reboot anyway, hence there wouldn't be
much difference to the BUG() statement...
It's been a while now, but I think these BUGs were changed to WARNs due to
checkpatch.pl nagging. Not that I added the BUG statements, but my patch
moved them so checkpatch.pl felt that I owned them. Ahhh no, it was your
colleague Johannes Thumshirn.
The SCSI subsystem may not be the primary storage medium on a system, so
bringing down the system because the SCSI mid-level has an unrecoverable
error may be overkill. Think an embedded system with the rootfs on a
/dev/mmcblkp device with the only "SCSI" device being an external
USB port for a memory key (I was fighting with a 3D printer was just that
config a few days ago). Then the user plugs in the world's worst USB
key (billion of candidates). Isn't one error strategy just to report,
ignore and continue?
Doug Gilbert
Re: [PATCH v6 7/7] scsi_io_completion convert BUGs to WARNs
On 06/23/2018 12:22 PM, Douglas Gilbert wrote:
The scsi_io_completion function contains three BUG() and BUG_ON() calls.
Replace them with WARN variants.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
Reviewed-by: Bart Van Assche
---
drivers/scsi/scsi_lib.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 19ed11abe886..252edd61a688 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1060,13 +1060,21 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
scsi_req(req->next_rq)->resid_len = scsi_in(cmd)->resid;
if (scsi_end_request(req, BLK_STS_OK, blk_rq_bytes(req),
blk_rq_bytes(req->next_rq)))
- BUG();
+ WARN_ONCE(true,
+ "Bidi command with remaining bytes");
return;
}
}
/* no bidi support yet, other than in pass-through */
- BUG_ON(blk_bidi_rq(req));
+ if (unlikely(blk_bidi_rq(req))) {
+ WARN_ONCE(true, "Only support bidi command in passthrough");
+ scmd_printk(KERN_ERR, cmd, "Killing bidi command\n");
+ if (scsi_end_request(req, BLK_STS_IOERR, blk_rq_bytes(req),
+blk_rq_bytes(req->next_rq)))
+ WARN_ONCE(true, "Bidi command with remaining bytes");
+ return;
+ }
/*
* Next deal with any sectors which we were able to correctly
@@ -1089,7 +1097,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
/* Kill remainder if no retries. */
if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
- BUG();
+ WARN_ONCE(true,
+ "Bytes remaining after failed, no-retry command");
return;
}
So what happens to these requests if not all bytes could be finished?
Do we have an error recovery strategy?
Changing to WARN_ON would only makes sense if we have a way of
recovering the failed request, otherwise we'd have to reboot anyway,
hence there wouldn't be much difference to the BUG() statement...
Cheers,
Hannes
[PATCH v6 7/7] scsi_io_completion convert BUGs to WARNs
The scsi_io_completion function contains three BUG() and BUG_ON() calls.
Replace them with WARN variants.
Signed-off-by: Douglas Gilbert
Reviewed-by: Johannes Thumshirn
Reviewed-by: Bart Van Assche
---
drivers/scsi/scsi_lib.c | 15 ---
1 file changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 19ed11abe886..252edd61a688 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1060,13 +1060,21 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
scsi_req(req->next_rq)->resid_len = scsi_in(cmd)->resid;
if (scsi_end_request(req, BLK_STS_OK, blk_rq_bytes(req),
blk_rq_bytes(req->next_rq)))
- BUG();
+ WARN_ONCE(true,
+ "Bidi command with remaining bytes");
return;
}
}
/* no bidi support yet, other than in pass-through */
- BUG_ON(blk_bidi_rq(req));
+ if (unlikely(blk_bidi_rq(req))) {
+ WARN_ONCE(true, "Only support bidi command in passthrough");
+ scmd_printk(KERN_ERR, cmd, "Killing bidi command\n");
+ if (scsi_end_request(req, BLK_STS_IOERR, blk_rq_bytes(req),
+blk_rq_bytes(req->next_rq)))
+ WARN_ONCE(true, "Bidi command with remaining bytes");
+ return;
+ }
/*
* Next deal with any sectors which we were able to correctly
@@ -1089,7 +1097,8 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned
int good_bytes)
/* Kill remainder if no retries. */
if (unlikely(blk_stat && scsi_noretry_cmd(cmd))) {
if (scsi_end_request(req, blk_stat, blk_rq_bytes(req), 0))
- BUG();
+ WARN_ONCE(true,
+ "Bytes remaining after failed, no-retry command");
return;
}
--
2.17.1
