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
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer
