From: Damien Le Moal <Damien.LeMoal@wdc.com>
To: Finn Thain <fthain@telegraphics.com.au>
Cc: "linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
"Martin K . Petersen" <martin.petersen@oracle.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
"usb-storage@lists.one-eyed-alien.net"
<usb-storage@lists.one-eyed-alien.net>,
Alan Stern <stern@rowland.harvard.edu>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
Justin Piszcz <jpiszcz@lucidpixels.com>
Subject: Re: [PATCH V2] scsi: save/restore command resid for error handling
Date: Tue, 1 Oct 2019 02:09:43 +0000 [thread overview]
Message-ID: <BYAPR04MB5816FBEE392BE5E4BCF14B96E79D0@BYAPR04MB5816.namprd04.prod.outlook.com> (raw)
In-Reply-To: alpine.LNX.2.21.1910011011410.13@nippy.intranet
On 2019/09/30 17:42, Finn Thain wrote:
> On Sat, 28 Sep 2019, Damien Le Moal wrote:
>
>> When a non-passthrough command is terminated with CHECK CONDITION,
>> request sense is executed by hijacking the command descriptor. Since
>> scsi_eh_prep_cmnd() and scsi_eh_restore_cmnd() do not save/restore the
>> original command resid, the value returned on failure of the original
>> command is lost and replaced with the value set by the execution of the
>> request sense command. This value may in many instances be unaligned to
>> the device sector size, causing sd_done() to print a warning message
>> about the incorrect unaligned resid before the command is retried or
>> aborted.
>>
>> Fix this problem by saving the original command resid in struct
>> scsi_eh_save using scsi_eh_prep_cmnd() and restoring it in
>> scsi_eh_restore_cmnd(). In addition, to make sure that the request sense
>> command is executed with a correctly initialized command structure, also
>> reset resid to 0 in scsi_eh_prep_cmnd() after saving the original
>> command resid value in struct scsi_eh_save.
>>
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>
>> Changes from V1:
>> * Dropped patch 2
>> * Add resid reset in scsi_eh_prep_cmnd()
>>
>> drivers/scsi/scsi_error.c | 3 +++
>> include/scsi/scsi_eh.h | 1 +
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
>> index 1c470e31ae81..f53828bf7ad7 100644
>> --- a/drivers/scsi/scsi_error.c
>> +++ b/drivers/scsi/scsi_error.c
>> @@ -967,6 +967,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
>> ses->data_direction = scmd->sc_data_direction;
>> ses->sdb = scmd->sdb;
>> ses->result = scmd->result;
>> + ses->resid = scsi_get_resid(scmd);
>> ses->underflow = scmd->underflow;
>> ses->prot_op = scmd->prot_op;
>> ses->eh_eflags = scmd->eh_eflags;
>> @@ -977,6 +978,7 @@ void scsi_eh_prep_cmnd(struct scsi_cmnd *scmd, struct scsi_eh_save *ses,
>> memset(scmd->cmnd, 0, BLK_MAX_CDB);
>> memset(&scmd->sdb, 0, sizeof(scmd->sdb));
>> scmd->result = 0;
>> + scsi_set_resid(scmd, 0);
>>
>> if (sense_bytes) {
>> scmd->sdb.length = min_t(unsigned, SCSI_SENSE_BUFFERSIZE,
>> @@ -1029,6 +1031,7 @@ void scsi_eh_restore_cmnd(struct scsi_cmnd* scmd, struct scsi_eh_save *ses)
>> scmd->sc_data_direction = ses->data_direction;
>> scmd->sdb = ses->sdb;
>> scmd->result = ses->result;
>> + scsi_set_resid(scmd, ses->resid);
>
> When saving and restoring state, perhaps it makes more sense to bypass the
> higher level getter/setter API? Open-coded assignment statements are
> already prevalent here, rather than calls to e.g. scsi_set_prot_op(),
> set_msg_byte() etc. (There may be no code elsewhere that could tell the
> difference, but we can't use "private" members to prove it, unlike C++.)
>
>> scmd->underflow = ses->underflow;
>> scmd->prot_op = ses->prot_op;
>> scmd->eh_eflags = ses->eh_eflags;
>> diff --git a/include/scsi/scsi_eh.h b/include/scsi/scsi_eh.h
>> index 3810b340551c..9caa9b262a32 100644
>> --- a/include/scsi/scsi_eh.h
>> +++ b/include/scsi/scsi_eh.h
>> @@ -32,6 +32,7 @@ extern int scsi_ioctl_reset(struct scsi_device *, int __user *);
>> struct scsi_eh_save {
>> /* saved state */
>> int result;
>> + unsigned int resid;
>
> There seems to be an inconsistency here. A signed int would be consistent
> with the getter and setter helpers. Whereas, if you open-coded the
> assignments instead, your unsigned int would make sense because
> scsi_request.resid_len really is an unsigned int.
All fair points. Sending a V3. Thanks !
--
Damien Le Moal
Western Digital Research
prev parent reply other threads:[~2019-10-01 2:09 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-27 22:16 [PATCH V2] scsi: save/restore command resid for error handling Damien Le Moal
2019-10-01 0:42 ` Finn Thain
2019-10-01 2:09 ` Damien Le Moal [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BYAPR04MB5816FBEE392BE5E4BCF14B96E79D0@BYAPR04MB5816.namprd04.prod.outlook.com \
--to=damien.lemoal@wdc.com \
--cc=fthain@telegraphics.com.au \
--cc=gregkh@linuxfoundation.org \
--cc=jpiszcz@lucidpixels.com \
--cc=linux-scsi@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=stern@rowland.harvard.edu \
--cc=usb-storage@lists.one-eyed-alien.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).