Bart,
On 2/16/17 11:52, Damien Le Moal wrote:
> Bart,
>
> On 2/16/17 10:10, Bart Van Assche wrote:
>> On Thu, 2017-02-16 at 09:54 +0900, Damien Le Moal wrote:
>>> On 2/16/17 01:42, Bart Van Assche wrote:
>>>> An additional concern: what if the size of the Data-Out buffer is not a
>>>> multiple of the logical block size? Shouldn't we round down (good_bytes -
>>>> resid) instead of rounding up resid?
>>>
>>> The only REQ_TYPE_FS request that may not operate on LBA size aligned
>>> payloads is REQ_OP_ZONE_REPORT which is handles in a different case of
>>> the switch statement. So I think it is safe.
>>
>> Hello Damien,
>>
>> Are you aware that it is the software that submits a request that controls
>> the buffer length and not the device that processes a request? Submitting
>> Data-Out buffers with odd sizes is one of the tests in the libiscsi test
>> suite. See e.g. the source file test-tool/test_write10_residuals.c in that
>> test suite. The request submitted by the libiscsi test suite reach the
>> kernel of the target system either through SG IO or through the iSCSI
>> target driver. When using iSCSI, both the LIO and SCST SCSI target
>> frameworks translate these requests into REQ_TYPE_FS requests. I think we
>> should aim not to affect the outcome of libiscsi tests when the underlying
>> device is a SCSI disk.
>
> Thanks for the pointers. I checked libiscsi tests. And from what is done
> there, it seems to me that it is basically impossible to distinguished
> between a buggy hardware response and an in-purpose (or buggy) not
> aligned data-out buffer size.
> E.g. the test in test_write10_residuals.c which issues a 1 block write
> with a buf size of 10000 and checks that residual is 10000 - block size.
> For that case, with both this patch and the original mpt3sas patch, the
> rounding up of resid to the block size will break the test.
>
> Yet, it is very surprising that LIO and SCST_SCSI issue the "buggy"
> request as is without checking and eventually correcting things first.
> So unless LIO and SCST_SCSI are fixed to do that on the request they
> send to the device, both patches should be dropped.
> Users with buggy mpt3 HBAs will need to update the HBA FW...
>
> So the conclusion may be that we need to drop everything ? The mpt3sas
> patch breaks ZBC now, so that must be removed too.
>
> Thoughts ?
Or we rewrite to this:
if (!(good_bytes & (sector_size - 1)) &&
resid & (sector_size - 1)) {
resid = min(good_bytes, round_up(resid, sector_size));
good_bytes -= resid;
scsi_set_resid(SCpnt, resid);
}
That is, do the resid correction only and only if the command buffer
size is aligned. But still not sure that is safe in all possible cases.
--
Damien Le Moal, Ph.D.
Sr. Manager, System Software Research Group,
Western Digital Corporation
Damien.LeMoal@xxxxxxx
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.wdc.com, www.hgst.com
