| Message ID | 20180623102220.5438-8-dgilbert@interlog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 <dgilbert@interlog.com> > Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> > Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com> > --- > 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
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 <dgilbert@interlog.com> >> Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de> >> Reviewed-by: Bart Van Assche <bart.vanassche@wdc.com> >> --- >> 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/mmcblk<n>p<m> 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
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/mmcblk<n>p<m> 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
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; }