* [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
@ 2019-02-19 7:27 ` Jason Yan
0 siblings, 0 replies; 11+ messages in thread
From: Jason Yan @ 2019-02-19 7:27 UTC (permalink / raw)
To: martin.petersen, jejb
Cc: linux-scsi, linux-kernel, hare, dan.j.williams, jthumshirn, hch,
Jason Yan
If we remove the scsi disk when running io with fio, oops occured with
the following condition.
[scsi_eh_0] [fio]
scsi_end_request
->blk_update_request
->end_bio(io returned to userspace)
close
->sd_release
->scsi_disk_put
->scsi_disk_release
->disk->private_data = NULL;
->scsi_mq_uninit_cmd
->scsi_uninit_cmd
->scsi_cmd_to_driver
->drv is NULL, Oops
There is a small window between blk_update_request() and
scsi_mq_uninit_cmd() that scsi disk may have been released. This will
cause a oops like below:
Unable to handle kernel NULL pointer dereference at virtual address
0000000000000000
s/sync.c:67, func=xfer, error=In[11347.116050] Mem abort info:
put/output error
[11347.121598] ESR = 0x96000006
[11347.126200] Exception class = DABT (current EL), IL = 32 bits
[11347.132117] SET = 0, FnV = 0
[11347.135170] EA = 0, S1PTW = 0
[11347.138308] Data abort info:
[11347.141186] ISV = 0, ISS = 0x00000006
[11347.145019] CM = 0, WnR = 0
[11347.147977] user pgtable: 4k pages, 48-bit VAs, pgdp =
00000000a67aece2
[11347.154591] [0000000000000000] pgd=0000002f90774003,
pud=0000002fab098003, pmd=0000000000000000
[11347.163304] Internal error: Oops: 96000006 [#1] PREEMPT SMP
[11347.168870] Modules linked in: hisi_sas_v3_hw hisi_sas_main libsas
[11347.175044] CPU: 56 PID: 4294 Comm: scsi_eh_2 Not tainted
4.19.0-g8052059-dirty #2
[11347.182600] Hardware name: Huawei D06/D06, BIOS Hisilicon D06 UEFI
RC0 - B601 (V6.01) 11/08/2018
[11347.191370] pstate: a0c00009 (NzCv daif +PAN +UAO)
[11347.196155] pc : scsi_uninit_cmd+0x24/0x3c
[11347.200240] lr : scsi_mq_uninit_cmd+0x1c/0x30
[11347.204583] sp : ffff000024dabb60
[11347.207884] x29: ffff000024dabb60 x28: ffff000024dabd38
[11347.213184] x27: ffff000000f5b3a8 x26: ffff7df3b0181600
[11347.218484] x25: 0000000000000000 x24: ffff803bc5d36778
[11347.223783] x23: 000000000000000a x22: 0000000000000000
[11347.229082] x21: ffff803bc7397000 x20: ffff802f9148e530
[11347.234381] x19: ffff802f9148e530 x18: ffff7e0000000000
[11347.239679] x17: 0000000000000000 x16: 0000002f9e37d000
[11347.244979] x15: ffff7e0000000000 x14: 3863206336203839
[11347.250278] x13: 2036302030302038 x12: a46fac3d0d363d00
[11347.255578] x11: ffffffffffffffff x10: a46fac3d0d363d00
[11347.260877] x9 : 0000000040040000 x8 : 000000000000eb4b
[11347.266177] x7 : ffff000009771000 x6 : 0000000000210d00
[11347.271476] x5 : ffff803bc9f50000 x4 : 0000000000000000
[11347.276775] x3 : ffff802fb02b4380 x2 : ffff802f9148e400
[11347.282075] x1 : 0000000000000000 x0 : ffff802f9148e530
[11347.287375] Process scsi_eh_2 (pid: 4294, stack limit =
0x000000007d2257f8)
[11347.294323] Call trace:
Jobs: 6 (f=6): [R[RRR1XXX1XRR3] 47.296758] scsi_uninit_cmd+0x24/0x3c
[22.7% done] [1516MB/0KB/0KB /s] [754/0/0 iops] [eta 08m:39s]
[11347.308390] scsi_mq_uninit_cmd+0x1c/0x30
[11347.312387] scsi_end_request+0x7c/0x1b8
[11347.316297] scsi_io_completion+0x464/0x668
[11347.320467] scsi_finish_command+0xbc/0x160
[11347.324636] scsi_eh_flush_done_q+0x10c/0x170
[11347.328990] sas_scsi_recover_host+0x84c/0xa98 [libsas]
[11347.334202] scsi_error_handler+0x140/0x5b0
[11347.338374] kthread+0x100/0x12c
[11347.341590] ret_from_fork+0x10/0x18
[11347.345153] Code: 71000c3f 540000e9 f9404c41 f941f421 (f9400021)
[11347.351234] ---[ end trace f496aacdaa1dcc51 ]---
To fix this, get a refcount of scsi_disk in sd_init_command() to ensure
it will not be released before sd_uninit_command().
Signed-off-by: Jason Yan <yanaijie@huawei.com>
---
drivers/scsi/sd.c | 46 +++++++++++++++++++++++++++++++++++-----------
1 file changed, 35 insertions(+), 11 deletions(-)
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 5464d467e23e..6bdb8fbb570f 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -1249,42 +1249,64 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
static blk_status_t sd_init_command(struct scsi_cmnd *cmd)
{
struct request *rq = cmd->request;
+ struct scsi_disk *sdkp = NULL;
+ blk_status_t ret;
switch (req_op(rq)) {
case REQ_OP_DISCARD:
switch (scsi_disk(rq->rq_disk)->provisioning_mode) {
case SD_LBP_UNMAP:
- return sd_setup_unmap_cmnd(cmd);
+ ret = sd_setup_unmap_cmnd(cmd);
+ break;
case SD_LBP_WS16:
- return sd_setup_write_same16_cmnd(cmd, true);
+ ret = sd_setup_write_same16_cmnd(cmd, true);
+ break;
case SD_LBP_WS10:
- return sd_setup_write_same10_cmnd(cmd, true);
+ ret = sd_setup_write_same10_cmnd(cmd, true);
+ break;
case SD_LBP_ZERO:
- return sd_setup_write_same10_cmnd(cmd, false);
+ ret = sd_setup_write_same10_cmnd(cmd, false);
+ break;
default:
- return BLK_STS_TARGET;
+ ret = BLK_STS_TARGET;
+ break;
}
+ break;
case REQ_OP_WRITE_ZEROES:
- return sd_setup_write_zeroes_cmnd(cmd);
+ ret = sd_setup_write_zeroes_cmnd(cmd);
+ break;
case REQ_OP_WRITE_SAME:
- return sd_setup_write_same_cmnd(cmd);
+ ret = sd_setup_write_same_cmnd(cmd);
+ break;
case REQ_OP_FLUSH:
- return sd_setup_flush_cmnd(cmd);
+ ret = sd_setup_flush_cmnd(cmd);
+ break;
case REQ_OP_READ:
case REQ_OP_WRITE:
- return sd_setup_read_write_cmnd(cmd);
+ ret = sd_setup_read_write_cmnd(cmd);
+ break;
case REQ_OP_ZONE_RESET:
- return sd_zbc_setup_reset_cmnd(cmd);
+ ret = sd_zbc_setup_reset_cmnd(cmd);
+ break;
default:
WARN_ON_ONCE(1);
- return BLK_STS_NOTSUPP;
+ ret = BLK_STS_NOTSUPP;
+ break;
}
+
+ if (!ret) {
+ sdkp = scsi_disk(rq->rq_disk);
+ get_device(&sdkp->dev);
+ }
+
+ return ret;
}
static void sd_uninit_command(struct scsi_cmnd *SCpnt)
{
struct request *rq = SCpnt->request;
u8 *cmnd;
+ struct scsi_disk *sdkp = NULL;
if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
mempool_free(rq->special_vec.bv_page, sd_page_pool);
@@ -1295,6 +1317,8 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
SCpnt->cmd_len = 0;
mempool_free(cmnd, sd_cdb_pool);
}
+ sdkp = scsi_disk(rq->rq_disk);
+ put_device(&sdkp->dev);
}
/**
--
2.14.4
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
2019-02-19 7:27 ` Jason Yan
(?)
@ 2019-02-19 10:32 ` Steffen Maier
-1 siblings, 0 replies; 11+ messages in thread
From: Steffen Maier @ 2019-02-19 10:32 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb
Cc: linux-scsi, linux-kernel, hare, dan.j.williams, jthumshirn, hch
On 02/19/2019 08:27 AM, Jason Yan wrote:
> If we remove the scsi disk when running io with fio, oops occured with
> the following condition.
>
> [scsi_eh_0] [fio]
> scsi_end_request
> ->blk_update_request
> ->end_bio(io returned to userspace)
> close
> ->sd_release
> ->scsi_disk_put
> ->scsi_disk_release
> ->disk->private_data = NULL;
>
> ->scsi_mq_uninit_cmd
> ->scsi_uninit_cmd
> ->scsi_cmd_to_driver
> ->drv is NULL, Oops
>
> There is a small window between blk_update_request() and
> scsi_mq_uninit_cmd() that scsi disk may have been released. This will
> cause a oops like below:
> To fix this, get a refcount of scsi_disk in sd_init_command() to ensure
> it will not be released before sd_uninit_command().
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
> drivers/scsi/sd.c | 46 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5464d467e23e..6bdb8fbb570f 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1249,42 +1249,64 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
> static blk_status_t sd_init_command(struct scsi_cmnd *cmd)
> {
> struct request *rq = cmd->request;
> + struct scsi_disk *sdkp = NULL;
This pre-init with NULL kinda prevents static compile warnings on uninitialized
use?
> + blk_status_t ret;
>
> switch (req_op(rq)) {
> }
> +
> + if (!ret) {
> + sdkp = scsi_disk(rq->rq_disk);
> + get_device(&sdkp->dev);
> + }
> +
> + return ret;
> }
>
> static void sd_uninit_command(struct scsi_cmnd *SCpnt)
> {
> struct request *rq = SCpnt->request;
> u8 *cmnd;
> + struct scsi_disk *sdkp = NULL;
dito
>
> if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
> mempool_free(rq->special_vec.bv_page, sd_page_pool);
> @@ -1295,6 +1317,8 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
> SCpnt->cmd_len = 0;
> mempool_free(cmnd, sd_cdb_pool);
> }
> + sdkp = scsi_disk(rq->rq_disk);
> + put_device(&sdkp->dev);
> }
>
> /**
>
--
Mit freundlichen Gruessen / Kind regards
Steffen Maier
Linux on IBM Z Development
https://www.ibm.com/privacy/us/en/
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Matthias Hartmann
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
2019-02-19 7:27 ` Jason Yan
(?)
(?)
@ 2019-02-19 16:56 ` Bart Van Assche
2019-02-20 15:18 ` Christoph Hellwig
-1 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2019-02-19 16:56 UTC (permalink / raw)
To: Jason Yan, martin.petersen, jejb, Jens Axboe, Christoph Hellwig
Cc: linux-scsi, linux-kernel, hare, dan.j.williams, jthumshirn,
Steffen Maier
On Tue, 2019-02-19 at 15:27 +0800, Jason Yan wrote:
> If we remove the scsi disk when running io with fio, oops occured with
> the following condition.
>
> [scsi_eh_0] [fio]
> scsi_end_request
> ->blk_update_request
> ->end_bio(io returned to userspace)
> close
> ->sd_release
> ->scsi_disk_put
> ->scsi_disk_release
> ->disk->private_data = NULL;
>
> ->scsi_mq_uninit_cmd
> ->scsi_uninit_cmd
> ->scsi_cmd_to_driver
> ->drv is NULL, Oops
>
> There is a small window between blk_update_request() and
> scsi_mq_uninit_cmd() that scsi disk may have been released. This will
> cause a oops like below:
>
> Unable to handle kernel NULL pointer dereference at virtual address
> 0000000000000000
> s/sync.c:67, func=xfer, error=In[11347.116050] Mem abort info:
> put/output error
> [11347.121598] ESR = 0x96000006
> [11347.126200] Exception class = DABT (current EL), IL = 32 bits
> [11347.132117] SET = 0, FnV = 0
> [11347.135170] EA = 0, S1PTW = 0
> [11347.138308] Data abort info:
> [11347.141186] ISV = 0, ISS = 0x00000006
> [11347.145019] CM = 0, WnR = 0
> [11347.147977] user pgtable: 4k pages, 48-bit VAs, pgdp =
> 00000000a67aece2
> [11347.154591] [0000000000000000] pgd=0000002f90774003,
> pud=0000002fab098003, pmd=0000000000000000
> [11347.163304] Internal error: Oops: 96000006 [#1] PREEMPT SMP
> [11347.168870] Modules linked in: hisi_sas_v3_hw hisi_sas_main libsas
> [11347.175044] CPU: 56 PID: 4294 Comm: scsi_eh_2 Not tainted
> 4.19.0-g8052059-dirty #2
> [11347.182600] Hardware name: Huawei D06/D06, BIOS Hisilicon D06 UEFI
> RC0 - B601 (V6.01) 11/08/2018
> [11347.191370] pstate: a0c00009 (NzCv daif +PAN +UAO)
> [11347.196155] pc : scsi_uninit_cmd+0x24/0x3c
> [11347.200240] lr : scsi_mq_uninit_cmd+0x1c/0x30
> [11347.204583] sp : ffff000024dabb60
> [11347.207884] x29: ffff000024dabb60 x28: ffff000024dabd38
> [11347.213184] x27: ffff000000f5b3a8 x26: ffff7df3b0181600
> [11347.218484] x25: 0000000000000000 x24: ffff803bc5d36778
> [11347.223783] x23: 000000000000000a x22: 0000000000000000
> [11347.229082] x21: ffff803bc7397000 x20: ffff802f9148e530
> [11347.234381] x19: ffff802f9148e530 x18: ffff7e0000000000
> [11347.239679] x17: 0000000000000000 x16: 0000002f9e37d000
> [11347.244979] x15: ffff7e0000000000 x14: 3863206336203839
> [11347.250278] x13: 2036302030302038 x12: a46fac3d0d363d00
> [11347.255578] x11: ffffffffffffffff x10: a46fac3d0d363d00
> [11347.260877] x9 : 0000000040040000 x8 : 000000000000eb4b
> [11347.266177] x7 : ffff000009771000 x6 : 0000000000210d00
> [11347.271476] x5 : ffff803bc9f50000 x4 : 0000000000000000
> [11347.276775] x3 : ffff802fb02b4380 x2 : ffff802f9148e400
> [11347.282075] x1 : 0000000000000000 x0 : ffff802f9148e530
> [11347.287375] Process scsi_eh_2 (pid: 4294, stack limit =
> 0x000000007d2257f8)
> [11347.294323] Call trace:
> Jobs: 6 (f=6): [R[RRR1XXX1XRR3] 47.296758] scsi_uninit_cmd+0x24/0x3c
> [22.7% done] [1516MB/0KB/0KB /s] [754/0/0 iops] [eta 08m:39s]
> [11347.308390] scsi_mq_uninit_cmd+0x1c/0x30
> [11347.312387] scsi_end_request+0x7c/0x1b8
> [11347.316297] scsi_io_completion+0x464/0x668
> [11347.320467] scsi_finish_command+0xbc/0x160
> [11347.324636] scsi_eh_flush_done_q+0x10c/0x170
> [11347.328990] sas_scsi_recover_host+0x84c/0xa98 [libsas]
> [11347.334202] scsi_error_handler+0x140/0x5b0
> [11347.338374] kthread+0x100/0x12c
> [11347.341590] ret_from_fork+0x10/0x18
> [11347.345153] Code: 71000c3f 540000e9 f9404c41 f941f421 (f9400021)
> [11347.351234] ---[ end trace f496aacdaa1dcc51 ]---
>
> To fix this, get a refcount of scsi_disk in sd_init_command() to ensure
> it will not be released before sd_uninit_command().
>
> Signed-off-by: Jason Yan <yanaijie@huawei.com>
> ---
> drivers/scsi/sd.c | 46 +++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 5464d467e23e..6bdb8fbb570f 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -1249,42 +1249,64 @@ static blk_status_t sd_setup_read_write_cmnd(struct scsi_cmnd *SCpnt)
> static blk_status_t sd_init_command(struct scsi_cmnd *cmd)
> {
> struct request *rq = cmd->request;
> + struct scsi_disk *sdkp = NULL;
> + blk_status_t ret;
>
> switch (req_op(rq)) {
> case REQ_OP_DISCARD:
> switch (scsi_disk(rq->rq_disk)->provisioning_mode) {
> case SD_LBP_UNMAP:
> - return sd_setup_unmap_cmnd(cmd);
> + ret = sd_setup_unmap_cmnd(cmd);
> + break;
> case SD_LBP_WS16:
> - return sd_setup_write_same16_cmnd(cmd, true);
> + ret = sd_setup_write_same16_cmnd(cmd, true);
> + break;
> case SD_LBP_WS10:
> - return sd_setup_write_same10_cmnd(cmd, true);
> + ret = sd_setup_write_same10_cmnd(cmd, true);
> + break;
> case SD_LBP_ZERO:
> - return sd_setup_write_same10_cmnd(cmd, false);
> + ret = sd_setup_write_same10_cmnd(cmd, false);
> + break;
> default:
> - return BLK_STS_TARGET;
> + ret = BLK_STS_TARGET;
> + break;
> }
> + break;
> case REQ_OP_WRITE_ZEROES:
> - return sd_setup_write_zeroes_cmnd(cmd);
> + ret = sd_setup_write_zeroes_cmnd(cmd);
> + break;
> case REQ_OP_WRITE_SAME:
> - return sd_setup_write_same_cmnd(cmd);
> + ret = sd_setup_write_same_cmnd(cmd);
> + break;
> case REQ_OP_FLUSH:
> - return sd_setup_flush_cmnd(cmd);
> + ret = sd_setup_flush_cmnd(cmd);
> + break;
> case REQ_OP_READ:
> case REQ_OP_WRITE:
> - return sd_setup_read_write_cmnd(cmd);
> + ret = sd_setup_read_write_cmnd(cmd);
> + break;
> case REQ_OP_ZONE_RESET:
> - return sd_zbc_setup_reset_cmnd(cmd);
> + ret = sd_zbc_setup_reset_cmnd(cmd);
> + break;
> default:
> WARN_ON_ONCE(1);
> - return BLK_STS_NOTSUPP;
> + ret = BLK_STS_NOTSUPP;
> + break;
> }
> +
> + if (!ret) {
> + sdkp = scsi_disk(rq->rq_disk);
> + get_device(&sdkp->dev);
> + }
> +
> + return ret;
> }
>
> static void sd_uninit_command(struct scsi_cmnd *SCpnt)
> {
> struct request *rq = SCpnt->request;
> u8 *cmnd;
> + struct scsi_disk *sdkp = NULL;
>
> if (rq->rq_flags & RQF_SPECIAL_PAYLOAD)
> mempool_free(rq->special_vec.bv_page, sd_page_pool);
> @@ -1295,6 +1317,8 @@ static void sd_uninit_command(struct scsi_cmnd *SCpnt)
> SCpnt->cmd_len = 0;
> mempool_free(cmnd, sd_cdb_pool);
> }
> + sdkp = scsi_disk(rq->rq_disk);
> + put_device(&sdkp->dev);
> }
Hi Jens and Christoph,
My interpretation of the above bug report and patch is that this is a
regression in the SCSI sd driver due to the switch from the legacy block
layer to scsi-mq. The above patch introduces two atomic operations in the
hot path and hence would introduce a performance regression. I think this
can be avoided by making sure that sd_uninit_command() gets called before
the request tag is freed. What changes would be required to make the block
layer core call sd_uninit_command() before the request tag is freed? Would
introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
making sure that the SCSI core sets these callback function pointers
appropriately be sufficient? Would such a change allow to simplify the NVMe
initiator driver? Are there any alternatives to this approach that are more
elegant?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
2019-02-19 16:56 ` Bart Van Assche
@ 2019-02-20 15:18 ` Christoph Hellwig
2019-02-21 8:53 ` Jason Yan
0 siblings, 1 reply; 11+ messages in thread
From: Christoph Hellwig @ 2019-02-20 15:18 UTC (permalink / raw)
To: Bart Van Assche
Cc: Jason Yan, martin.petersen, jejb, Jens Axboe, Christoph Hellwig,
linux-scsi, linux-kernel, hare, dan.j.williams, jthumshirn,
Steffen Maier
[fullquote removed, please follow proper mail etiquette]
On Tue, Feb 19, 2019 at 08:56:28AM -0800, Bart Van Assche wrote:
> regression in the SCSI sd driver due to the switch from the legacy block
> layer to scsi-mq. The above patch introduces two atomic operations in the
> hot path and hence would introduce a performance regression. I think this
> can be avoided by making sure that sd_uninit_command() gets called before
> the request tag is freed. What changes would be required to make the block
> layer core call sd_uninit_command() before the request tag is freed? Would
> introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
> making sure that the SCSI core sets these callback function pointers
> appropriately be sufficient? Would such a change allow to simplify the NVMe
> initiator driver? Are there any alternatives to this approach that are more
> elegant?
Additional indirect calls in the I/O fast path is something I'd rather
avoid. But I don't fully understand the problem yet - where do
we release a disk reference from blk_update_request? And why can't
we move that release to __blk_mq_end_request?
>
> Thanks,
>
> Bart.
---end quoted text---
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
2019-02-20 15:18 ` Christoph Hellwig
@ 2019-02-21 8:53 ` Jason Yan
0 siblings, 0 replies; 11+ messages in thread
From: Jason Yan @ 2019-02-21 8:53 UTC (permalink / raw)
To: Christoph Hellwig, Bart Van Assche
Cc: martin.petersen, jejb, Jens Axboe, linux-scsi, linux-kernel,
hare, dan.j.williams, jthumshirn, Steffen Maier
Hi, Christoph
On 2019/2/20 23:18, Christoph Hellwig wrote:
> [fullquote removed, please follow proper mail etiquette]
>
> On Tue, Feb 19, 2019 at 08:56:28AM -0800, Bart Van Assche wrote:
>> regression in the SCSI sd driver due to the switch from the legacy block
>> layer to scsi-mq. The above patch introduces two atomic operations in the
>> hot path and hence would introduce a performance regression. I think this
>> can be avoided by making sure that sd_uninit_command() gets called before
>> the request tag is freed. What changes would be required to make the block
>> layer core call sd_uninit_command() before the request tag is freed? Would
>> introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
>> making sure that the SCSI core sets these callback function pointers
>> appropriately be sufficient? Would such a change allow to simplify the NVMe
>> initiator driver? Are there any alternatives to this approach that are more
>> elegant?
>
> Additional indirect calls in the I/O fast path is something I'd rather
> avoid. But I don't fully understand the problem yet - where do
> we release a disk reference from blk_update_request?
When userspace close the fd after blk_update_request() and before
scsi_mq_uninit_cmd(), a disk reference will be released. It is not the
blk_update_request() directly released it.
close
->sd_release
->scsi_disk_put
->scsi_disk_release
->disk->private_data = NULL;
The userspace can close the fd because blk_update_request() returned the
last IO , the userspace application does not have to stuck on read() or
write(). The window is very small, but it can be reproduce every day
in our testcases. So I'm very curious why. One possible explanation is
that we enabled kernel preempt(CONFIG_PREEMPT).
And why can't
> we move that release to __blk_mq_end_request?
>
>>
>> Thanks,
>>
>> Bart.
> ---end quoted text---
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
@ 2019-02-21 8:53 ` Jason Yan
0 siblings, 0 replies; 11+ messages in thread
From: Jason Yan @ 2019-02-21 8:53 UTC (permalink / raw)
To: Christoph Hellwig, Bart Van Assche
Cc: martin.petersen, jejb, Jens Axboe, linux-scsi, linux-kernel,
hare, dan.j.williams, jthumshirn, Steffen Maier
Hi, Christoph
On 2019/2/20 23:18, Christoph Hellwig wrote:
> [fullquote removed, please follow proper mail etiquette]
>
> On Tue, Feb 19, 2019 at 08:56:28AM -0800, Bart Van Assche wrote:
>> regression in the SCSI sd driver due to the switch from the legacy block
>> layer to scsi-mq. The above patch introduces two atomic operations in the
>> hot path and hence would introduce a performance regression. I think this
>> can be avoided by making sure that sd_uninit_command() gets called before
>> the request tag is freed. What changes would be required to make the block
>> layer core call sd_uninit_command() before the request tag is freed? Would
>> introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
>> making sure that the SCSI core sets these callback function pointers
>> appropriately be sufficient? Would such a change allow to simplify the NVMe
>> initiator driver? Are there any alternatives to this approach that are more
>> elegant?
>
> Additional indirect calls in the I/O fast path is something I'd rather
> avoid. But I don't fully understand the problem yet - where do
> we release a disk reference from blk_update_request?
When userspace close the fd after blk_update_request() and before
scsi_mq_uninit_cmd(), a disk reference will be released. It is not the
blk_update_request() directly released it.
close
->sd_release
->scsi_disk_put
->scsi_disk_release
->disk->private_data = NULL;
The userspace can close the fd because blk_update_request() returned the
last IO , the userspace application does not have to stuck on read() or
write(). The window is very small, but it can be reproduce every day
in our testcases. So I'm very curious why. One possible explanation is
that we enabled kernel preempt(CONFIG_PREEMPT).
And why can't
> we move that release to __blk_mq_end_request?
>
>>
>> Thanks,
>>
>> Bart.
> ---end quoted text---
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
2019-02-21 8:53 ` Jason Yan
(?)
@ 2019-03-13 23:51 ` Bart Van Assche
2019-03-14 1:57 ` Jason Yan
-1 siblings, 1 reply; 11+ messages in thread
From: Bart Van Assche @ 2019-03-13 23:51 UTC (permalink / raw)
To: Jason Yan, Christoph Hellwig
Cc: martin.petersen, jejb, Jens Axboe, linux-scsi, linux-kernel,
hare, dan.j.williams, jthumshirn, Steffen Maier
On Thu, 2019-02-21 at 16:53 +0800, Jason Yan wrote:
> On 2019/2/20 23:18, Christoph Hellwig wrote:
> > [fullquote removed, please follow proper mail etiquette]
> >
> > On Tue, Feb 19, 2019 at 08:56:28AM -0800, Bart Van Assche wrote:
> > > regression in the SCSI sd driver due to the switch from the legacy block
> > > layer to scsi-mq. The above patch introduces two atomic operations in the
> > > hot path and hence would introduce a performance regression. I think this
> > > can be avoided by making sure that sd_uninit_command() gets called before
> > > the request tag is freed. What changes would be required to make the block
> > > layer core call sd_uninit_command() before the request tag is freed? Would
> > > introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
> > > making sure that the SCSI core sets these callback function pointers
> > > appropriately be sufficient? Would such a change allow to simplify the NVMe
> > > initiator driver? Are there any alternatives to this approach that are more
> > > elegant?
> >
> > Additional indirect calls in the I/O fast path is something I'd rather
> > avoid. But I don't fully understand the problem yet - where do
> > we release a disk reference from blk_update_request?
>
> When userspace close the fd after blk_update_request() and before
> scsi_mq_uninit_cmd(), a disk reference will be released. It is not the
> blk_update_request() directly released it.
>
> close
> ->sd_release
> ->scsi_disk_put
> ->scsi_disk_release
> ->disk->private_data = NULL;
>
> The userspace can close the fd because blk_update_request() returned the
> last IO , the userspace application does not have to stuck on read() or
> write(). The window is very small, but it can be reproduce every day
> in our testcases. So I'm very curious why. One possible explanation is
> that we enabled kernel preempt(CONFIG_PREEMPT).
>
> And why can't we move that release to __blk_mq_end_request?
Hi Jason,
What is the current status of this issue?
Thanks,
Bart.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
2019-03-13 23:51 ` Bart Van Assche
@ 2019-03-14 1:57 ` Jason Yan
0 siblings, 0 replies; 11+ messages in thread
From: Jason Yan @ 2019-03-14 1:57 UTC (permalink / raw)
To: Bart Van Assche, Christoph Hellwig
Cc: martin.petersen, jejb, Jens Axboe, linux-scsi, linux-kernel,
hare, dan.j.williams, jthumshirn, Steffen Maier
On 2019/3/14 7:51, Bart Van Assche wrote:
> On Thu, 2019-02-21 at 16:53 +0800, Jason Yan wrote:
>> On 2019/2/20 23:18, Christoph Hellwig wrote:
>>> [fullquote removed, please follow proper mail etiquette]
>>>
>>> On Tue, Feb 19, 2019 at 08:56:28AM -0800, Bart Van Assche wrote:
>>>> regression in the SCSI sd driver due to the switch from the legacy block
>>>> layer to scsi-mq. The above patch introduces two atomic operations in the
>>>> hot path and hence would introduce a performance regression. I think this
>>>> can be avoided by making sure that sd_uninit_command() gets called before
>>>> the request tag is freed. What changes would be required to make the block
>>>> layer core call sd_uninit_command() before the request tag is freed? Would
>>>> introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
>>>> making sure that the SCSI core sets these callback function pointers
>>>> appropriately be sufficient? Would such a change allow to simplify the NVMe
>>>> initiator driver? Are there any alternatives to this approach that are more
>>>> elegant?
>>>
>>> Additional indirect calls in the I/O fast path is something I'd rather
>>> avoid. But I don't fully understand the problem yet - where do
>>> we release a disk reference from blk_update_request?
>>
>> When userspace close the fd after blk_update_request() and before
>> scsi_mq_uninit_cmd(), a disk reference will be released. It is not the
>> blk_update_request() directly released it.
>>
>> close
>> ->sd_release
>> ->scsi_disk_put
>> ->scsi_disk_release
>> ->disk->private_data = NULL;
>>
>> The userspace can close the fd because blk_update_request() returned the
>> last IO , the userspace application does not have to stuck on read() or
>> write(). The window is very small, but it can be reproduce every day
>> in our testcases. So I'm very curious why. One possible explanation is
>> that we enabled kernel preempt(CONFIG_PREEMPT).
>>
>> And why can't we move that release to __blk_mq_end_request?
>
> Hi Jason,
>
> What is the current status of this issue?
>
Hi Bart,
I did not find any other approach that will not affect the hot path. I
don't know if you guys have other suggestions?
> Thanks,
>
> Bart.
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
@ 2019-03-14 1:57 ` Jason Yan
0 siblings, 0 replies; 11+ messages in thread
From: Jason Yan @ 2019-03-14 1:57 UTC (permalink / raw)
To: Bart Van Assche, Christoph Hellwig
Cc: martin.petersen, jejb, Jens Axboe, linux-scsi, linux-kernel,
hare, dan.j.williams, jthumshirn, Steffen Maier
On 2019/3/14 7:51, Bart Van Assche wrote:
> On Thu, 2019-02-21 at 16:53 +0800, Jason Yan wrote:
>> On 2019/2/20 23:18, Christoph Hellwig wrote:
>>> [fullquote removed, please follow proper mail etiquette]
>>>
>>> On Tue, Feb 19, 2019 at 08:56:28AM -0800, Bart Van Assche wrote:
>>>> regression in the SCSI sd driver due to the switch from the legacy block
>>>> layer to scsi-mq. The above patch introduces two atomic operations in the
>>>> hot path and hence would introduce a performance regression. I think this
>>>> can be avoided by making sure that sd_uninit_command() gets called before
>>>> the request tag is freed. What changes would be required to make the block
>>>> layer core call sd_uninit_command() before the request tag is freed? Would
>>>> introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
>>>> making sure that the SCSI core sets these callback function pointers
>>>> appropriately be sufficient? Would such a change allow to simplify the NVMe
>>>> initiator driver? Are there any alternatives to this approach that are more
>>>> elegant?
>>>
>>> Additional indirect calls in the I/O fast path is something I'd rather
>>> avoid. But I don't fully understand the problem yet - where do
>>> we release a disk reference from blk_update_request?
>>
>> When userspace close the fd after blk_update_request() and before
>> scsi_mq_uninit_cmd(), a disk reference will be released. It is not the
>> blk_update_request() directly released it.
>>
>> close
>> ->sd_release
>> ->scsi_disk_put
>> ->scsi_disk_release
>> ->disk->private_data = NULL;
>>
>> The userspace can close the fd because blk_update_request() returned the
>> last IO , the userspace application does not have to stuck on read() or
>> write(). The window is very small, but it can be reproduce every day
>> in our testcases. So I'm very curious why. One possible explanation is
>> that we enabled kernel preempt(CONFIG_PREEMPT).
>>
>> And why can't we move that release to __blk_mq_end_request?
>
> Hi Jason,
>
> What is the current status of this issue?
>
Hi Bart,
I did not find any other approach that will not affect the hot path. I
don't know if you guys have other suggestions?
> Thanks,
>
> Bart.
>
> .
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [RFC PATCH] scsi: fix oops in scsi_uninit_cmd()
2019-02-21 8:53 ` Jason Yan
(?)
(?)
@ 2019-03-15 7:56 ` Ming Lei
-1 siblings, 0 replies; 11+ messages in thread
From: Ming Lei @ 2019-03-15 7:56 UTC (permalink / raw)
To: Jason Yan
Cc: Christoph Hellwig, Bart Van Assche, Martin K. Petersen,
James Bottomley, Jens Axboe, Linux SCSI List,
Linux Kernel Mailing List, Hannes Reinecke, Dan Williams,
Johannes Thumshirn, Steffen Maier
On Thu, Feb 21, 2019 at 4:55 PM Jason Yan <yanaijie@huawei.com> wrote:
>
> Hi, Christoph
>
> On 2019/2/20 23:18, Christoph Hellwig wrote:
> > [fullquote removed, please follow proper mail etiquette]
> >
> > On Tue, Feb 19, 2019 at 08:56:28AM -0800, Bart Van Assche wrote:
> >> regression in the SCSI sd driver due to the switch from the legacy block
> >> layer to scsi-mq. The above patch introduces two atomic operations in the
> >> hot path and hence would introduce a performance regression. I think this
> >> can be avoided by making sure that sd_uninit_command() gets called before
> >> the request tag is freed. What changes would be required to make the block
> >> layer core call sd_uninit_command() before the request tag is freed? Would
> >> introducing prep_rq_fn and unprep_rq_fn callbacks in struct blk_mq_ops and
> >> making sure that the SCSI core sets these callback function pointers
> >> appropriately be sufficient? Would such a change allow to simplify the NVMe
> >> initiator driver? Are there any alternatives to this approach that are more
> >> elegant?
> >
> > Additional indirect calls in the I/O fast path is something I'd rather
> > avoid. But I don't fully understand the problem yet - where do
> > we release a disk reference from blk_update_request?
>
> When userspace close the fd after blk_update_request() and before
> scsi_mq_uninit_cmd(), a disk reference will be released. It is not the
> blk_update_request() directly released it.
>
> close
> ->sd_release
> ->scsi_disk_put
> ->scsi_disk_release
> ->disk->private_data = NULL;
>
> The userspace can close the fd because blk_update_request() returned the
> last IO , the userspace application does not have to stuck on read() or
> write(). The window is very small, but it can be reproduce every day
> in our testcases. So I'm very curious why. One possible explanation is
> that we enabled kernel preempt(CONFIG_PREEMPT).
Another solution is to drain in-flight FS IO in scsi_disk_release(),
and one counter
is needed for tracking in-flight passthrough IO, so we can use sdev->device_busy
- sdev->passthrough_ios to drain inflight FS IO.
>
> And why can't
> > we move that release to __blk_mq_end_request?
I think it is doable, then ending bio needs to be moved out of
blk_update_request(),
such as, add one list of rq->done_bio to track completed bio, then complete all
in free request.
And for partial completion, the completed bio can be done in
blk_update_request(),
since the remained bios will cause fs to hold the disk.
Thanks,
Ming Lei
^ permalink raw reply [flat|nested] 11+ messages in thread