Re: [PATCH 14/42] scsi: add scsi_result_is_good()

Hannes Reinecke
2021-04-22T08:43:07Z

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