| Message ID | 20210421174749.11221-15-hare@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | SCSI result cleanup, part 2 | expand |
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 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 -- Dr. Hannes Reinecke Kernel Storage Architect hare@suse.de +49 911 74053 688 SUSE Software Solutions Germany GmbH, 90409 Nürnberg GF: F. Imendörffer, HRB 36809 (AG Nürnberg)
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(+)