| Message ID | 20210421174749.11221-15-hare@suse.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | SCSI result cleanup, part 2 | expand |
On 4/21/21 10:47 AM, Hannes Reinecke wrote: > +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd) > +{ > + return (cmd->result == 0); > +} Do we really need an inline function to compare an integer with zero? How about open-coding this comparison in the callers of this function? Thanks, Bart.
On 2021-04-21 5:10 p.m., Bart Van Assche wrote: > On 4/21/21 10:47 AM, Hannes Reinecke wrote: >> +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd) >> +{ >> + return (cmd->result == 0); >> +} > > Do we really need an inline function to compare an integer with zero? How about > open-coding this comparison in the callers of this function? > Please don't open code it. Please fix it! Spot the difference: static inline int scsi_status_is_good(int status) { /* * FIXME: bit0 is listed as reserved in SCSI-2, but is * significant in SCSI-3. For now, we follow the SCSI-2 * behaviour and ignore reserved bits. */ status &= 0xfe; return ((status == SAM_STAT_GOOD) || (status == SAM_STAT_CONDITION_MET) || /* >>> ^^^^^^^^^^^^^^^^^^^^^^ <<< */ /* Next two "intermediate" statuses are obsolete in SAM-4 */ (status == SAM_STAT_INTERMEDIATE) || (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) || /* FIXME: this is obsolete in SAM-3 */ (status == SAM_STAT_COMMAND_TERMINATED)); } In sg3_utils' library I ignore the last three SAM_STATs. Not sure if ignoring bit 0 is still required. Without considering SAM_STAT_CONDITION_MET a good status, someone will be scratching their head wondering why so many PRE-FETCH commands fail. That command can be used when a sequence of READs to consecutive LBAs is followed by a different (i.e. non-consecutive) READ. That last READ could be preceded by PRE-FETCH(new_LBA, IMMED). Assuming there is processing of the data from the consecutive LBAs to be done, when the time comes for READ(new_LBA) the probability of its data being in the disk's cache is increased. Doug Gilbert
On 4/21/21 11:10 PM, Bart Van Assche wrote: > On 4/21/21 10:47 AM, Hannes Reinecke wrote: >> +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd) >> +{ >> + return (cmd->result == 0); >> +} > > Do we really need an inline function to compare an integer with zero? > How about open-coding this comparison in the callers of this function? > My approach is to avoid direct access to the 'result' field, as the definition of which is about to change. But as this is not part of _this_ patchset I'll drop this patch for the next round. Cheers, Hannes
On 4/21/21 11:58 PM, Douglas Gilbert wrote: > On 2021-04-21 5:10 p.m., Bart Van Assche wrote: >> On 4/21/21 10:47 AM, Hannes Reinecke wrote: >>> +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd) >>> +{ >>> + return (cmd->result == 0); >>> +} >> >> Do we really need an inline function to compare an integer with zero? >> How about open-coding this comparison in the callers of this function? >> > > Please don't open code it. Please fix it! > Spot the difference: > > static inline int scsi_status_is_good(int status) > { > /* > * FIXME: bit0 is listed as reserved in SCSI-2, but is > * significant in SCSI-3. For now, we follow the SCSI-2 > * behaviour and ignore reserved bits. > */ > status &= 0xfe; > return ((status == SAM_STAT_GOOD) || > (status == SAM_STAT_CONDITION_MET) || > /* >>> ^^^^^^^^^^^^^^^^^^^^^^ > <<< */ > /* Next two "intermediate" statuses are obsolete in > SAM-4 */ > (status == SAM_STAT_INTERMEDIATE) || > (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) || > /* FIXME: this is obsolete in SAM-3 */ > (status == SAM_STAT_COMMAND_TERMINATED)); > } > > In sg3_utils' library I ignore the last three SAM_STATs. Not sure if > ignoring > bit 0 is still required. > > Without considering SAM_STAT_CONDITION_MET a good status, someone will be > scratching their head wondering why so many PRE-FETCH commands fail. > > That command can be used when a sequence of READs to consecutive LBAs is > followed by a different (i.e. non-consecutive) READ. That last READ could > be preceded by PRE-FETCH(new_LBA, IMMED). Assuming there is processing > of the data from the consecutive LBAs to be done, when the time comes > for READ(new_LBA) the probability of its data being in the disk's cache is > increased. > That would be a change in behaviour. Current code doesn't check for CONDITION_MET, so this change shouldn't do it, neither. Idea was that this patchset shouldn't change the current behaviour. While your argument might be valid, it definitely is a different story and would need to be address with a different patchset. Cheers, Hannes
On Thu, 22 Apr 2021, Hannes Reinecke wrote: > That would be a change in behaviour. Current code doesn't check for > CONDITION_MET, so this change shouldn't do it, neither. Idea was that > this patchset shouldn't change the current behaviour. > > While your argument might be valid, it definitely is a different story > and would need to be address with a different patchset. > As long as you're avoiding behavioural changes, you may need to drop the status_byte() change in patch 15/42 from this particular patch set -- unless it can be shown (inferred somehow) that drives never set that bit.
On 2021-04-22 4:42 a.m., Hannes Reinecke wrote: > On 4/21/21 11:58 PM, Douglas Gilbert wrote: >> On 2021-04-21 5:10 p.m., Bart Van Assche wrote: >>> On 4/21/21 10:47 AM, Hannes Reinecke wrote: >>>> +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd) >>>> +{ >>>> + return (cmd->result == 0); >>>> +} >>> >>> Do we really need an inline function to compare an integer with zero? How >>> about open-coding this comparison in the callers of this function? >>> >> >> Please don't open code it. Please fix it! >> Spot the difference: >> >> static inline int scsi_status_is_good(int status) >> { >> /* >> * FIXME: bit0 is listed as reserved in SCSI-2, but is >> * significant in SCSI-3. For now, we follow the SCSI-2 >> * behaviour and ignore reserved bits. >> */ >> status &= 0xfe; >> return ((status == SAM_STAT_GOOD) || >> (status == SAM_STAT_CONDITION_MET) || >> /* >>> ^^^^^^^^^^^^^^^^^^^^^^ <<< */ >> /* Next two "intermediate" statuses are obsolete in SAM-4 */ >> (status == SAM_STAT_INTERMEDIATE) || >> (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) || >> /* FIXME: this is obsolete in SAM-3 */ >> (status == SAM_STAT_COMMAND_TERMINATED)); >> } >> >> In sg3_utils' library I ignore the last three SAM_STATs. Not sure if ignoring >> bit 0 is still required. >> >> Without considering SAM_STAT_CONDITION_MET a good status, someone will be >> scratching their head wondering why so many PRE-FETCH commands fail. >> >> That command can be used when a sequence of READs to consecutive LBAs is >> followed by a different (i.e. non-consecutive) READ. That last READ could >> be preceded by PRE-FETCH(new_LBA, IMMED). Assuming there is processing >> of the data from the consecutive LBAs to be done, when the time comes >> for READ(new_LBA) the probability of its data being in the disk's cache is >> increased. >> > That would be a change in behaviour. > Current code doesn't check for CONDITION_MET, so this change shouldn't do it, > neither. Idea was that this patchset shouldn't change the current behaviour. > > While your argument might be valid, it definitely is a different story and would > need to be address with a different patchset. Okay. May I suggest a "FIX_ME" comment? And again, please don't open code it. In driver manuals Seagate often list the PRE-FETCH command as optional. As in: pay us some extra money and we will put it in. That suggests to me some big OEM likes PRE-FETCH. Where I found it supported in WDC manuals, they didn't support the IMMED bit which sort of defeats the purpose of it IMO. Doug Gilbert
On 2021-04-22 11:56 a.m., Douglas Gilbert wrote: > On 2021-04-22 4:42 a.m., Hannes Reinecke wrote: >> On 4/21/21 11:58 PM, Douglas Gilbert wrote: >>> On 2021-04-21 5:10 p.m., Bart Van Assche wrote: >>>> On 4/21/21 10:47 AM, Hannes Reinecke wrote: >>>>> +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd) >>>>> +{ >>>>> + return (cmd->result == 0); >>>>> +} >>>> >>>> Do we really need an inline function to compare an integer with zero? How >>>> about open-coding this comparison in the callers of this function? >>>> >>> >>> Please don't open code it. Please fix it! >>> Spot the difference: >>> >>> static inline int scsi_status_is_good(int status) >>> { >>> /* >>> * FIXME: bit0 is listed as reserved in SCSI-2, but is >>> * significant in SCSI-3. For now, we follow the SCSI-2 >>> * behaviour and ignore reserved bits. >>> */ >>> status &= 0xfe; >>> return ((status == SAM_STAT_GOOD) || >>> (status == SAM_STAT_CONDITION_MET) || >>> /* >>> ^^^^^^^^^^^^^^^^^^^^^^ <<< */ >>> /* Next two "intermediate" statuses are obsolete in SAM-4 */ >>> (status == SAM_STAT_INTERMEDIATE) || >>> (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) || >>> /* FIXME: this is obsolete in SAM-3 */ >>> (status == SAM_STAT_COMMAND_TERMINATED)); >>> } >>> >>> In sg3_utils' library I ignore the last three SAM_STATs. Not sure if ignoring >>> bit 0 is still required. >>> >>> Without considering SAM_STAT_CONDITION_MET a good status, someone will be >>> scratching their head wondering why so many PRE-FETCH commands fail. >>> >>> That command can be used when a sequence of READs to consecutive LBAs is >>> followed by a different (i.e. non-consecutive) READ. That last READ could >>> be preceded by PRE-FETCH(new_LBA, IMMED). Assuming there is processing >>> of the data from the consecutive LBAs to be done, when the time comes >>> for READ(new_LBA) the probability of its data being in the disk's cache is >>> increased. >>> >> That would be a change in behaviour. >> Current code doesn't check for CONDITION_MET, so this change shouldn't do it, >> neither. Idea was that this patchset shouldn't change the current behaviour. >> >> While your argument might be valid, it definitely is a different story and >> would need to be address with a different patchset. > > Okay. May I suggest a "FIX_ME" comment? And again, please don't open code it. > > In driver manuals Seagate often list the PRE-FETCH command as optional. As > in: pay us some extra money and we will put it in. That suggests to me some > big OEM likes PRE-FETCH. Where I found it supported in WDC manuals, they > didn't support the IMMED bit which sort of defeats the purpose of it IMO. And PRE-FETCH has another (sneaky) use. You might think that when a LBA is unmapped, then if its data is in the cache, it would be removed. So what does SBC-4 say about reading unmapped LBAs (sbc4r22 4.7.4.4 table 10): "user data set to a vendor-specific value that is not obtained from any other LBA; and" (... place 0xff bytes in the PI) Spot the weasel word: _other_ ! So it can read the former data in that now unmapped LBA. So the second usage of PRE-FETCH is to prevent that. Even more worryingly, it looks like PRE-FETCH may be needed after a cryptographic erase (via the SANITIZE command)! SYNCHRONIZE CACHE only talks about the write side of the cache, not removing stale read data. Doug Gilbert
On 4/22/21 8:56 AM, Douglas Gilbert wrote: > In driver manuals Seagate often list the PRE-FETCH command as optional. As > in: pay us some extra money and we will put it in. That suggests to me some > big OEM likes PRE-FETCH. Where I found it supported in WDC manuals, they > didn't support the IMMED bit which sort of defeats the purpose of it IMO. Since the sd driver does not submit the PRE-FETCH command, how about moving support for CONDITION MET into the sg code and treating CONDITION MET as an error inside the sd, sr and st drivers? I think that would allow to simplify scsi_status_is_good(). The current definition of that function is as follows: static inline int scsi_status_is_good(int status) { /* * FIXME: bit0 is listed as reserved in SCSI-2, but is * significant in SCSI-3. For now, we follow the SCSI-2 * behaviour and ignore reserved bits. */ status &= 0xfe; return ((status == SAM_STAT_GOOD) || (status == SAM_STAT_CONDITION_MET) || /* Next two "intermediate" statuses are obsolete in*/ /* SAM-4 */ (status == SAM_STAT_INTERMEDIATE) || (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) || /* FIXME: this is obsolete in SAM-3 */ (status == SAM_STAT_COMMAND_TERMINATED)); } Thanks, Bart.
On 2021-04-22 12:52 p.m., Bart Van Assche wrote: > On 4/22/21 8:56 AM, Douglas Gilbert wrote: >> In driver manuals Seagate often list the PRE-FETCH command as optional. As >> in: pay us some extra money and we will put it in. That suggests to me some >> big OEM likes PRE-FETCH. Where I found it supported in WDC manuals, they >> didn't support the IMMED bit which sort of defeats the purpose of it IMO. > > Since the sd driver does not submit the PRE-FETCH command, how about > moving support for CONDITION MET into the sg code and treating CONDITION > MET as an error inside the sd, sr and st drivers? I think that would > allow to simplify scsi_status_is_good(). The current definition of that > function is as follows: > > static inline int scsi_status_is_good(int status) > { > /* > * FIXME: bit0 is listed as reserved in SCSI-2, but is > * significant in SCSI-3. For now, we follow the SCSI-2 > * behaviour and ignore reserved bits. > */ > status &= 0xfe; > return ((status == SAM_STAT_GOOD) || > (status == SAM_STAT_CONDITION_MET) || > /* Next two "intermediate" statuses are obsolete in*/ > /* SAM-4 */ > (status == SAM_STAT_INTERMEDIATE) || > (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) || > /* FIXME: this is obsolete in SAM-3 */ > (status == SAM_STAT_COMMAND_TERMINATED)); > } The whole stack needs to treat SAM_STAT_CONDITION_MET as a non-error. However the complex multi-layer return values are represented, reducing them to a comparison with zero, spread all over the stack just seems bad software engineering. IMO a predicate function (i.e. returning bool) is needed. I would argue that in the right circumstances, the sd driver should indeed by using PRE-FETCH. It would need help from the upper layers. It is essentially "read-ahead" in the case where the next LBA does not follow the last read LBA. A smarter read-ahead ... Doug Gilbert
On 4/22/21 7:33 PM, Douglas Gilbert wrote: > On 2021-04-22 12:52 p.m., Bart Van Assche wrote: >> On 4/22/21 8:56 AM, Douglas Gilbert wrote: >>> In driver manuals Seagate often list the PRE-FETCH command as >>> optional. As >>> in: pay us some extra money and we will put it in. That suggests to >>> me some >>> big OEM likes PRE-FETCH. Where I found it supported in WDC manuals, they >>> didn't support the IMMED bit which sort of defeats the purpose of it >>> IMO. >> >> Since the sd driver does not submit the PRE-FETCH command, how about >> moving support for CONDITION MET into the sg code and treating CONDITION >> MET as an error inside the sd, sr and st drivers? I think that would >> allow to simplify scsi_status_is_good(). The current definition of that >> function is as follows: >> >> static inline int scsi_status_is_good(int status) >> { >> /* >> * FIXME: bit0 is listed as reserved in SCSI-2, but is >> * significant in SCSI-3. For now, we follow the SCSI-2 >> * behaviour and ignore reserved bits. >> */ >> status &= 0xfe; >> return ((status == SAM_STAT_GOOD) || >> (status == SAM_STAT_CONDITION_MET) || >> /* Next two "intermediate" statuses are obsolete in*/ >> /* SAM-4 */ >> (status == SAM_STAT_INTERMEDIATE) || >> (status == SAM_STAT_INTERMEDIATE_CONDITION_MET) || >> /* FIXME: this is obsolete in SAM-3 */ >> (status == SAM_STAT_COMMAND_TERMINATED)); >> } > > The whole stack needs to treat SAM_STAT_CONDITION_MET as a non-error. > However the complex multi-layer return values are represented, > reducing them to a comparison with zero, spread all over the > stack just seems bad software engineering. IMO a predicate function > (i.e. returning bool) is needed. > > I would argue that in the right circumstances, the sd driver should > indeed by using PRE-FETCH. It would need help from the upper layers. > It is essentially "read-ahead" in the case where the next LBA > does not follow the last read LBA. A smarter read-ahead ... > Might, but again not something we should attempt in this patchset. Using PRE-FETCH might be worthwhile for larger I/O, which we could easily prepend with a PRE-FETCH command for the entire range. But then error handling for PRE-FETCH is decidedly tricky, and we might end up incurring quite some regressions just because we didn't get the error handling right in the first go. Worth a shot, but please in another patchset. The other use-case for using PRE-FETCH after cryptographic erase definitely is required, but as that's triggered by userspace I would expect userspace to do it properly, too. The only valid use-case I see would be for issuing PRE-FETCH after UNMAP, but that would need to be plugged into the 'discard' machinery, which is already fragile as hell; I'd rather not attempt that one. Cheers, Hannes
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 0ac18a7d8ac6..7089617911e1 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -336,6 +336,10 @@ static inline unsigned char get_host_byte(struct scsi_cmnd *cmd) return (cmd->result >> 16) & 0xff; } +static inline bool scsi_result_is_good(struct scsi_cmnd *cmd) +{ + return (cmd->result == 0); +} static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd) {
Add helper to check if the status is 'GOOD', ie if none of the status bytes are set. Signed-off-by: Hannes Reinecke <hare@suse.de> --- include/scsi/scsi_cmnd.h | 4 ++++ 1 file changed, 4 insertions(+)