On Tue, 2019-04-09 at 16:29 -0700, Jaesoo Lee wrote: +AD4 Let me comment in line. +AD4 +AD4 On Tue, Apr 9, 2019 at 3:14 PM Bart Van Assche +ADw-bvanassche+AEA-acm.org+AD4 wrote: +AD4 +AD4 +AD4 +AD4 On Tue, 2019-04-09 at 14:53 -0700, Jaesoo Lee wrote: +AD4 +AD4 +AD4 When SCSI blk-mq is enabled, there is a bug in handling errors in scsi+AF8-queue+AF8-rq. +AD4 +AD4 +AD4 Specifically, the bug is not setting result field of scsi+AF8-request correctly when +AD4 +AD4 +AD4 the dispatch of the command has been failed. Since the upper layer code +AD4 +AD4 +AD4 including the sg+AF8-io ioctl expects to receive any error status from result field +AD4 +AD4 +AD4 of scsi+AF8-request, the error is silently ignored and this could cause data +AD4 +AD4 +AD4 corruptions for some applications. This commit also fixes another bug that the +AD4 +AD4 +AD4 result field is not initialized when scsi+AF8-request is allocated. +AD4 +AD4 +AD4 +AD4 +AD4 +AD4 Signed-off-by: Jaesoo Lee +ADw-jalee+AEA-purestorage.com+AD4 +AD4 +AD4 +AD4 --- +AD4 +AD4 +AD4 block/scsi+AF8-ioctl.c +AHw 1 +- +AD4 +AD4 +AD4 drivers/scsi/scsi+AF8-lib.c +AHw 1 +- +AD4 +AD4 +AD4 2 files changed, 2 insertions(+-) +AD4 +AD4 +AD4 +AD4 +AD4 +AD4 diff --git a/block/scsi+AF8-ioctl.c b/block/scsi+AF8-ioctl.c +AD4 +AD4 +AD4 index 533f4ae..f2d7979 100644 +AD4 +AD4 +AD4 --- a/block/scsi+AF8-ioctl.c +AD4 +AD4 +AD4 +-+-+- b/block/scsi+AF8-ioctl.c +AD4 +AD4 +AD4 +AEAAQA -723,6 +-723,7 +AEAAQA void scsi+AF8-req+AF8-init(struct scsi+AF8-request +ACo-req) +AD4 +AD4 +AD4 req-+AD4-cmd +AD0 req-+AD4AXwBf-cmd+ADs +AD4 +AD4 +AD4 req-+AD4-cmd+AF8-len +AD0 BLK+AF8-MAX+AF8-CDB+ADs +AD4 +AD4 +AD4 req-+AD4-sense+AF8-len +AD0 0+ADs +AD4 +AD4 +AD4 +- req-+AD4-result +AD0 0+ADs +AD4 +AD4 +AD4 +AH0 +AD4 +AD4 +AD4 EXPORT+AF8-SYMBOL(scsi+AF8-req+AF8-init)+ADs +AD4 +AD4 +AD4 +AD4 What makes you think that this assignment is necessary? +AD4 +AD4 +AD4 +AD4 Actually, I discovered this before fixing this bug and we might not +AD4 see this problem anymore once this bug is fixed. +AD4 +AD4 Previously, since we are not setting scsi+AF8-req(req)-+AD4-result in +AD4 scsi+AF8-queue+AF8-rq, I found that the application could receive another +AD4 DID+AF8-TRANSPORT+AF8-DISRUPTED host+AF8-status again if the same 'struct request' +AD4 is allocated for the IO. +AD4 +AD4 Please let me know if I need to remove this change. Since SCSI LLDs have to set that result variable anyway if a request completes successfully I'd prefer not to add that assignment. +AD4 +AD4 +AD4 diff --git a/drivers/scsi/scsi+AF8-lib.c b/drivers/scsi/scsi+AF8-lib.c +AD4 +AD4 +AD4 index 2018967..af1488d 100644 +AD4 +AD4 +AD4 --- a/drivers/scsi/scsi+AF8-lib.c +AD4 +AD4 +AD4 +-+-+- b/drivers/scsi/scsi+AF8-lib.c +AD4 +AD4 +AD4 +AEAAQA -1699,6 +-1699,7 +AEAAQA static blk+AF8-status+AF8-t scsi+AF8-queue+AF8-rq(struct +AD4 +AD4 +AD4 blk+AF8-mq+AF8-hw+AF8-ctx +ACo-hctx, +AD4 +AD4 +AD4 ret +AD0 BLK+AF8-STS+AF8-DEV+AF8-RESOURCE+ADs +AD4 +AD4 +AD4 break+ADs +AD4 +AD4 +AD4 default: +AD4 +AD4 +AD4 +- scsi+AF8-req(req)-+AD4-result +AD0 DID+AF8-NO+AF8-CONNECT +ADwAPA 16+ADs +AD4 +AD4 +AD4 /+ACo +AD4 +AD4 +AD4 +ACo Make sure to release all allocated ressources when +AD4 +AD4 +AD4 +ACo we hit an error, as we will never see this command +AD4 +AD4 +AD4 +AD4 What leads you to the conclusion that (ret +ACEAPQ BLK+AF8-STS+AF8-OK +ACYAJg +AD4 +AD4 ret +ACEAPQ BLK+AF8-STS+AF8-RESOUCE) means that there is a connectivity issue? +AD4 +AD4 I found this is what we are doing for legacy queue case+ADs I referred to +AD4 scsi+AF8-prep+AF8-return() and scsi+AF8-kill+AF8-request() code where we always +AD4 returning DID+AF8-NO+AF8-CONNECT. +AD4 +AD4 However, I think proper return code handling should be something like: +AD4 +AD4 diff --git a/drivers/scsi/scsi+AF8-lib.c b/drivers/scsi/scsi+AF8-lib.c +AD4 index 2018967..21e516e 100644 +AD4 --- a/drivers/scsi/scsi+AF8-lib.c +AD4 +-+-+- b/drivers/scsi/scsi+AF8-lib.c +AD4 +AEAAQA -1699,6 +-1699,10 +AEAAQA static blk+AF8-status+AF8-t scsi+AF8-queue+AF8-rq(struct +AD4 blk+AF8-mq+AF8-hw+AF8-ctx +ACo-hctx, +AD4 ret +AD0 BLK+AF8-STS+AF8-DEV+AF8-RESOURCE+ADs +AD4 break+ADs +AD4 default: +AD4 +- if (unlikely(+ACE-scsi+AF8-device+AF8-online(sdev))) +AD4 +- scsi+AF8-req(req)-+AD4-result +AD0 DID+AF8-NO+AF8-CONNECT +ADwAPA 16+ADs +AD4 +- else +AD4 +- scsi+AF8-req(req)-+AD4-result +AD0 DID+AF8-ERROR +ADwAPA 16+ADs +AD4 /+ACo +AD4 +ACo Make sure to release all allocated ressources when +AD4 +ACo we hit an error, as we will never see this command The above looks better to me than the original patch. Thanks, Bart.
