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
--
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
