| Message ID | 20240809193115.1222737-2-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix system resume for SCSI devices | expand |
On 8/10/24 04:31, Bart Van Assche wrote: > The SCSI core does not retry passthrough commands even if the SCSI device > reports a retryable unit attention condition. Support retrying in this case > by introducing the SCMD_RETRY_PASSTHROUGH flag. > > Cc: Damien Le Moal <dlemoal@kernel.org> > Cc: Mike Christie <michael.christie@oracle.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/scsi_error.c | 5 ++++- > include/scsi/scsi_cmnd.h | 5 ++++- > 2 files changed, 8 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c > index 612489afe8d2..9089d39c2170 100644 > --- a/drivers/scsi/scsi_error.c > +++ b/drivers/scsi/scsi_error.c > @@ -1855,7 +1855,10 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd) > * assume caller has checked sense and determined > * the check condition was retryable. > */ > - if (req->cmd_flags & REQ_FAILFAST_DEV || blk_rq_is_passthrough(req)) > + if (req->cmd_flags & REQ_FAILFAST_DEV) > + return true; > + if (blk_rq_is_passthrough(req) && > + !(scmd->flags & SCMD_RETRY_PASSTHROUGH)) > return true; > > return false; return (req->cmd_flags & REQ_FAILFAST_DEV) || (blk_rq_is_passthrough(req) && !(scmd->flags & SCMD_RETRY_PASSTHROUGH)); maybe ? Not necessarily more readable though. > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index 45c40d200154..19c57446ab23 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -58,8 +58,11 @@ struct scsi_pointer { > */ > #define SCMD_FORCE_EH_SUCCESS (1 << 3) > #define SCMD_FAIL_IF_RECOVERING (1 << 4) > +/* If set, retry a passthrough command in case of a unit attention. */ "...in case of error". There are other sense keys than unit attention and there is no code checking that this flag applies only to unit attention. With this comment fixed: Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > +#define SCMD_RETRY_PASSTHROUGH (1 << 5) > /* flags preserved across unprep / reprep */ > -#define SCMD_PRESERVED_FLAGS (SCMD_INITIALIZED | SCMD_FAIL_IF_RECOVERING) > +#define SCMD_PRESERVED_FLAGS \ > + (SCMD_INITIALIZED | SCMD_FAIL_IF_RECOVERING | SCMD_RETRY_PASSTHROUGH) > > /* for scmd->state */ > #define SCMD_STATE_COMPLETE 0
On 8/19/24 4:25 PM, Damien Le Moal wrote: > On 8/10/24 04:31, Bart Van Assche wrote: >> + if (blk_rq_is_passthrough(req) && >> + !(scmd->flags & SCMD_RETRY_PASSTHROUGH)) >> return true; >> >> return false; > > return (req->cmd_flags & REQ_FAILFAST_DEV) || > (blk_rq_is_passthrough(req) && > !(scmd->flags & SCMD_RETRY_PASSTHROUGH)); > > maybe ? Not necessarily more readable though. > >> diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h >> index 45c40d200154..19c57446ab23 100644 >> --- a/include/scsi/scsi_cmnd.h >> +++ b/include/scsi/scsi_cmnd.h >> @@ -58,8 +58,11 @@ struct scsi_pointer { >> */ >> #define SCMD_FORCE_EH_SUCCESS (1 << 3) >> #define SCMD_FAIL_IF_RECOVERING (1 << 4) >> +/* If set, retry a passthrough command in case of a unit attention. */ > > "...in case of error". There are other sense keys than unit attention and there > is no code checking that this flag applies only to unit attention. Hi Damien, Thanks for the detailed feedback. Since Martin replied to this series that he will reconcile his changes with mine, I plan to wait until either Martin posts his patches or until Martin asks me to repost my patches. Thanks, Bart.
Hi Bart! Sorry this took so long. I have made several attempts at automatically retrying commands originating from within SCSI since the lack of retries is a general problem. However, in the end I prefer the approach of describing the specific conditions we want handled using scsi_failure at each call site. Works well. > The SCSI core does not retry passthrough commands even if the SCSI > device reports a retryable unit attention condition. Support retrying > in this case by introducing the SCMD_RETRY_PASSTHROUGH flag. FWIW, I find the use of "passthrough" to describe commands issued by the SCSI layer quite confusing.
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c index 612489afe8d2..9089d39c2170 100644 --- a/drivers/scsi/scsi_error.c +++ b/drivers/scsi/scsi_error.c @@ -1855,7 +1855,10 @@ bool scsi_noretry_cmd(struct scsi_cmnd *scmd) * assume caller has checked sense and determined * the check condition was retryable. */ - if (req->cmd_flags & REQ_FAILFAST_DEV || blk_rq_is_passthrough(req)) + if (req->cmd_flags & REQ_FAILFAST_DEV) + return true; + if (blk_rq_is_passthrough(req) && + !(scmd->flags & SCMD_RETRY_PASSTHROUGH)) return true; return false; diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 45c40d200154..19c57446ab23 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -58,8 +58,11 @@ struct scsi_pointer { */ #define SCMD_FORCE_EH_SUCCESS (1 << 3) #define SCMD_FAIL_IF_RECOVERING (1 << 4) +/* If set, retry a passthrough command in case of a unit attention. */ +#define SCMD_RETRY_PASSTHROUGH (1 << 5) /* flags preserved across unprep / reprep */ -#define SCMD_PRESERVED_FLAGS (SCMD_INITIALIZED | SCMD_FAIL_IF_RECOVERING) +#define SCMD_PRESERVED_FLAGS \ + (SCMD_INITIALIZED | SCMD_FAIL_IF_RECOVERING | SCMD_RETRY_PASSTHROUGH) /* for scmd->state */ #define SCMD_STATE_COMPLETE 0
The SCSI core does not retry passthrough commands even if the SCSI device reports a retryable unit attention condition. Support retrying in this case by introducing the SCMD_RETRY_PASSTHROUGH flag. Cc: Damien Le Moal <dlemoal@kernel.org> Cc: Mike Christie <michael.christie@oracle.com> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/scsi_error.c | 5 ++++- include/scsi/scsi_cmnd.h | 5 ++++- 2 files changed, 8 insertions(+), 2 deletions(-)