On 01/04/2010 08:23 PM, Alan Stern wrote:
> On Mon, 4 Jan 2010, Boaz Harrosh wrote:
>
>> Embedding scsi_end_request() into scsi_io_completion actually simplifies the
>> code and makes it clearer what's going on.
>>
>> There is absolutely no functional and/or side effects changes after this patch.
>>
>> Patch was inspired by Alan Stern. (version 2 comments by Matthew Wilcox)
>
>> - /*
>> - * A number of bytes were successfully read. If there
>> - * are leftovers and there is some kind of error
>> - * (result != 0), retry the rest.
>> - */
>> - if (scsi_end_request(cmd, error, good_bytes, result == 0) == NULL)
>> - return;
>> -
>> - error = -EIO;
>> -
>> - if (host_byte(result) == DID_RESET) {
>> + if (likely(!blk_end_request(req, error, good_bytes))) {
>> + cmd->request = NULL;
>> + action = ACTION_NEXT_CMND;
>> + } else if (error && scsi_noretry_cmd(cmd)) {
>> + /* kill remainder if no retrys */
>> + blk_end_request_all(req, error);
>> + cmd->request = NULL;
>> + action = ACTION_NEXT_CMND;
>> + } else if (result == 0) {
>> + action = ACTION_REPREP;
>> + } else if (host_byte(result) == DID_RESET) {
>
> A few comments in these new "if" cases would help readers to understand
> the logic here.
>
> My personal preference is to reverse the order of the "if (error &&
> scsi_noretry_cmd(cmd))" and the "if (result == 0)" sections. It would
> make more sense; that way we'd have:
>
> If the request is finished [blk_end_request() == 0]
> ...
> else if the request was successful but has more work to do
> (result == 0)
> ...
> else if there was an error and retries are disallowed
> ...
> else ... [other error handling]
>
> Interchanging the order wouldn't make any functional difference,
> because the code above this point guarantees that we can never have
> result == 0 and error != 0.
>
> Apart from these small issues, it looks perfect.
>
> Alan Stern
>
Thanks Alan.
I agree with your comments and will send a new version.
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
