| Message ID | 20230621160111.1433521-1-yukuai1@huaweicloud.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | scsi/sg: don't grab scsi host module reference | expand |
On 6/21/23 09:01, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > In order to prevent request_queue to be freed before cleaning up > blktrace debugfs entries, commit db59133e9279 ("scsi: sg: fix blktrace > debugfs entries leakage") use scsi_device_get(), however, > scsi_device_get() will also grab scsi module reference and scsi module > can't be removed. > > It's reported that blktests can't unload scsi_debug after block/001: > > blktests (master) # ./check block > block/001 (stress device hotplugging) [failed] > +++ /root/blktests/results/nodev/block/001.out.bad 2023-06-19 > Running block/001 > Stressing sd > +modprobe: FATAL: Module scsi_debug is in use. > > Fix this problem by grabbing request_queue reference directly, so that > scsi host module can still be unloaded while request_queue will be > pinged by sg device. pinged -> pinned Otherwise this patch looks good to me. Bart.
On 6/21/23 09:01, Yu Kuai wrote: > From: Yu Kuai <yukuai3@huawei.com> > > In order to prevent request_queue to be freed before cleaning up > blktrace debugfs entries, commit db59133e9279 ("scsi: sg: fix blktrace > debugfs entries leakage") use scsi_device_get(), however, > scsi_device_get() will also grab scsi module reference and scsi module > can't be removed. I just noticed that this patch has been posted on the linux-scsi mailing list. If you plan to resend this patch, please send it to Jens and Cc both linux-block and linux-scsi because this patch fixes a bug in a patch that only exists in Jens' tree. Thanks, Bart.
Looks good:
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, 22 Jun 2023 00:01:11 +0800, Yu Kuai wrote: > In order to prevent request_queue to be freed before cleaning up > blktrace debugfs entries, commit db59133e9279 ("scsi: sg: fix blktrace > debugfs entries leakage") use scsi_device_get(), however, > scsi_device_get() will also grab scsi module reference and scsi module > can't be removed. > > It's reported that blktests can't unload scsi_debug after block/001: > > [...] Applied, thanks! [1/1] scsi/sg: don't grab scsi host module reference commit: fcaa174a9c995cf0af3967e55644a1543ea07e36 Best regards,
On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > From: Yu Kuai <yukuai3@huawei.com> > > In order to prevent request_queue to be freed before cleaning up > blktrace debugfs entries, commit db59133e9279 ("scsi: sg: fix blktrace > debugfs entries leakage") use scsi_device_get(), however, > scsi_device_get() will also grab scsi module reference and scsi module > can't be removed. > > It's reported that blktests can't unload scsi_debug after block/001: > > blktests (master) # ./check block > block/001 (stress device hotplugging) [failed] > +++ /root/blktests/results/nodev/block/001.out.bad 2023-06-19 > Running block/001 > Stressing sd > +modprobe: FATAL: Module scsi_debug is in use. > > Fix this problem by grabbing request_queue reference directly, so that > scsi host module can still be unloaded while request_queue will be > pinged by sg device. > > Reported-by: Chaitanya Kulkarni <chaitanyak@nvidia.com> > Link: https://lore.kernel.org/all/1760da91-876d-fc9c-ab51-999a6f66ad50@nvidia.com/ > Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage") > Signed-off-by: Yu Kuai <yukuai3@huawei.com> > --- > drivers/scsi/sg.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index 2433eeef042a..dcb73787c29d 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) > int error; > unsigned long iflags; > > - error = scsi_device_get(scsidp); > + error = blk_get_queue(scsidp->request_queue); > if (error) > return error; > > @@ -1558,7 +1558,7 @@ sg_add_device(struct device *cl_dev) > out: > if (cdev) > cdev_del(cdev); > - scsi_device_put(scsidp); > + blk_put_queue(scsidp->request_queue); > return error; > } > > @@ -1575,7 +1575,7 @@ sg_device_destroy(struct kref *kref) > */ > > blk_trace_remove(q); > - scsi_device_put(sdp->device); > + blk_put_queue(q); > > write_lock_irqsave(&sg_index_lock, flags); > idr_remove(&sg_index_idr, sdp->index); > -- > 2.39.2 Hi, This change (bisected) triggers a regression in our KVM on s390x CI. The symptom is that a “scsi_debug device” does not bind to the scsi_generic driver. On s390x you can reproduce the problem as follows (I have not tested on x86): With this patch applied: $ sudo modprobe scsi_debug $ # Get the 'scsi_host,channel,target_number,LUN' tuple for the scsi_debug device $ lsscsi |grep scsi_debug |awk '{ print $1 }' [0:0:0:0] $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic stat: cannot statx '/sys/bus/scsi/devices/0:0:0:0/scsi_generic': No such file or directory Patch reverted: $ sudo modprobe scsi_debug $ lsscsi |grep scsi_debug |awk '{ print $1 }' [0:0:0:0] $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic File: /sys/bus/scsi/devices/0:0:0:0/scsi_generic Size: 0 Blocks: 0 IO Block: 4096 directory Device: 0,20 Inode: 12155 Links: 3 … Any ideas? Marc
On Tue, Jul 04, 2023 at 07:04:00PM +0200, Marc Hartmayer wrote: > On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > > From: Yu Kuai <yukuai3@huawei.com> > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > > index 2433eeef042a..dcb73787c29d 100644 > > --- a/drivers/scsi/sg.c > > +++ b/drivers/scsi/sg.c > > @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) > > int error; > > unsigned long iflags; > > > > - error = scsi_device_get(scsidp); > > + error = blk_get_queue(scsidp->request_queue); > > if (error) > > return error; > > Might be interesting as well. Marc showed me a `dmesg` snipped earlier from when the bind fails: [ 15.441817] scsi host2: scsi_eh_2: sleeping [ 15.441899] scsi_debug:sdebug_driver_probe: scsi_debug: trim poll_queues to 0. poll_q/nr_hw = (0/1) [ 15.441907] scsi host2: scsi_debug: version 0191 [20210520] dev_size_mb=8, opts=0x0, submit_queues=1, statistics=0 [ 15.442078] scsi host2: scsi_scan_host_selected: <4294967295:4294967295:18446744073709551615> [ 15.442267] scsi 2:0:0:0: scsi scan: INQUIRY pass 1 length 36 [ 15.442286] scsi 2:0:0:0: scsi scan: INQUIRY successful with code 0x0 [ 15.442296] scsi 2:0:0:0: scsi scan: INQUIRY pass 2 length 96 [ 15.442308] scsi 2:0:0:0: scsi scan: INQUIRY successful with code 0x0 [ 15.442317] scsi 2:0:0:0: Direct-Access Linux scsi_debug 0191 PQ: 0 ANSI: 7 [ 15.442554] scsi 2:0:0:0: Power-on or device reset occurred [ 15.442560] scsi 2:0:0:0: tag#50 Done: SUCCESS Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s [ 15.442565] scsi 2:0:0:0: tag#50 CDB: Report supported operation codes a3 0c 01 88 00 00 00 00 00 14 00 00 [ 15.442569] scsi 2:0:0:0: tag#50 Sense Key : Unit Attention [current] [ 15.442573] scsi 2:0:0:0: tag#50 Add. Sense: Power on occurred The bind should happend around here somewhere I think. [ 15.472680] sd 2:0:0:0: scsi scan: Sending REPORT LUNS to (try 0) [ 15.472703] sd 2:0:0:0: scsi scan: REPORT LUNS successful (try 0) result 0x0 [ 15.472706] sd 2:0:0:0: scsi scan: REPORT LUN scan [ 15.472709] sd 2:0:0:0: scsi scan: device exists on 2:0:0:0 [ 15.492874] sd 2:0:0:0: [sdi] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB) [ 15.502853] sd 2:0:0:0: [sdi] Write Protect is off [ 15.502856] sd 2:0:0:0: [sdi] Mode Sense: 73 00 10 08 [ 15.522819] sd 2:0:0:0: [sdi] Write cache: enabled, read cache: enabled, supports DPO and FUA [ 15.552773] sd 2:0:0:0: [sdi] Preferred minimum I/O size 512 bytes [ 15.552776] sd 2:0:0:0: [sdi] Optimal transfer size 524288 bytes [ 15.575373] sd 2:0:0:0: [sdi] tag#62 Done: SUCCESS Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s [ 15.575377] sd 2:0:0:0: [sdi] tag#62 CDB: Inquiry 12 01 b9 00 04 00 [ 15.575380] sd 2:0:0:0: [sdi] tag#62 Sense Key : Illegal Request [current] [ 15.575383] sd 2:0:0:0: [sdi] tag#62 Add. Sense: Invalid field in cdb [ 15.645749] sd 2:0:0:0: [sdi] Attached SCSI disk But we don't even see the `sg_alloc: dev=...` message that is logged in `sg_alloc()`. And between the change above and the call to `sg_alloc()`, there is only the character device allocation; and if that failed, it would print an error. So either the bind is never even tried, or the new `blk_get_queue()` fails to get a reference. Which is odd, since the only way that would happen is, if the queue was marked dying; but we see that the stack is using it for LUN probing in `sd`. > This change (bisected) triggers a regression in our KVM on s390x CI. The > symptom is that a “scsi_debug device” does not bind to the scsi_generic > driver. On s390x you can reproduce the problem as follows (I have not > tested on x86): > > With this patch applied: > > $ sudo modprobe scsi_debug One more thing maybe worth mentioning: in the kernel configuration we use in the CI we have `sg` built-in. I guess most have it built as module. > $ # Get the 'scsi_host,channel,target_number,LUN' tuple for the scsi_debug device > $ lsscsi |grep scsi_debug |awk '{ print $1 }' > [0:0:0:0] > $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic > stat: cannot statx '/sys/bus/scsi/devices/0:0:0:0/scsi_generic': No such file or directory > > > Patch reverted: > > $ sudo modprobe scsi_debug > $ lsscsi |grep scsi_debug |awk '{ print $1 }' > [0:0:0:0] > $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic > File: /sys/bus/scsi/devices/0:0:0:0/scsi_generic > Size: 0 Blocks: 0 IO Block: 4096 directory > Device: 0,20 Inode: 12155 Links: 3 > … That's all I got from looking at it earlier, so far.
Hi, 在 2023/07/05 1:04, Marc Hartmayer 写道: > On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: >> From: Yu Kuai <yukuai3@huawei.com> >> >> In order to prevent request_queue to be freed before cleaning up >> blktrace debugfs entries, commit db59133e9279 ("scsi: sg: fix blktrace >> debugfs entries leakage") use scsi_device_get(), however, >> scsi_device_get() will also grab scsi module reference and scsi module >> can't be removed. >> >> It's reported that blktests can't unload scsi_debug after block/001: >> >> blktests (master) # ./check block >> block/001 (stress device hotplugging) [failed] >> +++ /root/blktests/results/nodev/block/001.out.bad 2023-06-19 >> Running block/001 >> Stressing sd >> +modprobe: FATAL: Module scsi_debug is in use. >> >> Fix this problem by grabbing request_queue reference directly, so that >> scsi host module can still be unloaded while request_queue will be >> pinged by sg device. >> >> Reported-by: Chaitanya Kulkarni <chaitanyak@nvidia.com> >> Link: https://lore.kernel.org/all/1760da91-876d-fc9c-ab51-999a6f66ad50@nvidia.com/ >> Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage") >> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >> --- >> drivers/scsi/sg.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >> index 2433eeef042a..dcb73787c29d 100644 >> --- a/drivers/scsi/sg.c >> +++ b/drivers/scsi/sg.c >> @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) >> int error; >> unsigned long iflags; >> >> - error = scsi_device_get(scsidp); >> + error = blk_get_queue(scsidp->request_queue); >> if (error) >> return error; >> >> @@ -1558,7 +1558,7 @@ sg_add_device(struct device *cl_dev) >> out: >> if (cdev) >> cdev_del(cdev); >> - scsi_device_put(scsidp); >> + blk_put_queue(scsidp->request_queue); >> return error; >> } >> >> @@ -1575,7 +1575,7 @@ sg_device_destroy(struct kref *kref) >> */ >> >> blk_trace_remove(q); >> - scsi_device_put(sdp->device); >> + blk_put_queue(q); >> >> write_lock_irqsave(&sg_index_lock, flags); >> idr_remove(&sg_index_idr, sdp->index); >> -- >> 2.39.2 > > Hi, > > This change (bisected) triggers a regression in our KVM on s390x CI. The > symptom is that a “scsi_debug device” does not bind to the scsi_generic > driver. On s390x you can reproduce the problem as follows (I have not > tested on x86): > > With this patch applied: > > $ sudo modprobe scsi_debug > $ # Get the 'scsi_host,channel,target_number,LUN' tuple for the scsi_debug device > $ lsscsi |grep scsi_debug |awk '{ print $1 }' > [0:0:0:0] > $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic > stat: cannot statx '/sys/bus/scsi/devices/0:0:0:0/scsi_generic': No such file or directory > > > Patch reverted: > I didn't figure out the root cause, howver, have you tried to reviert this patch as well? db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage" Thanks, Kuai > $ sudo modprobe scsi_debug > $ lsscsi |grep scsi_debug |awk '{ print $1 }' > [0:0:0:0] > $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic > File: /sys/bus/scsi/devices/0:0:0:0/scsi_generic > Size: 0 Blocks: 0 IO Block: 4096 directory > Device: 0,20 Inode: 12155 Links: 3 > … > > Any ideas? > > Marc > . >
Hi, 在 2023/07/05 2:51, Benjamin Block 写道: > On Tue, Jul 04, 2023 at 07:04:00PM +0200, Marc Hartmayer wrote: >> On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: >>> From: Yu Kuai <yukuai3@huawei.com> >>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >>> index 2433eeef042a..dcb73787c29d 100644 >>> --- a/drivers/scsi/sg.c >>> +++ b/drivers/scsi/sg.c >>> @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) >>> int error; >>> unsigned long iflags; >>> >>> - error = scsi_device_get(scsidp); >>> + error = blk_get_queue(scsidp->request_queue); >>> if (error) >>> return error; >>> > > Might be interesting as well. Marc showed me a `dmesg` snipped earlier > from when the bind fails: > > [ 15.441817] scsi host2: scsi_eh_2: sleeping > [ 15.441899] scsi_debug:sdebug_driver_probe: scsi_debug: trim poll_queues to 0. poll_q/nr_hw = (0/1) > [ 15.441907] scsi host2: scsi_debug: version 0191 [20210520] > dev_size_mb=8, opts=0x0, submit_queues=1, statistics=0 > [ 15.442078] scsi host2: scsi_scan_host_selected: <4294967295:4294967295:18446744073709551615> > [ 15.442267] scsi 2:0:0:0: scsi scan: INQUIRY pass 1 length 36 > [ 15.442286] scsi 2:0:0:0: scsi scan: INQUIRY successful with code 0x0 > [ 15.442296] scsi 2:0:0:0: scsi scan: INQUIRY pass 2 length 96 > [ 15.442308] scsi 2:0:0:0: scsi scan: INQUIRY successful with code 0x0 > [ 15.442317] scsi 2:0:0:0: Direct-Access Linux scsi_debug 0191 PQ: 0 ANSI: 7 > [ 15.442554] scsi 2:0:0:0: Power-on or device reset occurred > [ 15.442560] scsi 2:0:0:0: tag#50 Done: SUCCESS Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s > [ 15.442565] scsi 2:0:0:0: tag#50 CDB: Report supported operation codes a3 0c 01 88 00 00 00 00 00 14 00 00 > [ 15.442569] scsi 2:0:0:0: tag#50 Sense Key : Unit Attention [current] > [ 15.442573] scsi 2:0:0:0: tag#50 Add. Sense: Power on occurred > > The bind should happend around here somewhere I think. > > [ 15.472680] sd 2:0:0:0: scsi scan: Sending REPORT LUNS to (try 0) > [ 15.472703] sd 2:0:0:0: scsi scan: REPORT LUNS successful (try 0) result 0x0 > [ 15.472706] sd 2:0:0:0: scsi scan: REPORT LUN scan > [ 15.472709] sd 2:0:0:0: scsi scan: device exists on 2:0:0:0 > [ 15.492874] sd 2:0:0:0: [sdi] 16384 512-byte logical blocks: (8.39 MB/8.00 MiB) > [ 15.502853] sd 2:0:0:0: [sdi] Write Protect is off > [ 15.502856] sd 2:0:0:0: [sdi] Mode Sense: 73 00 10 08 > [ 15.522819] sd 2:0:0:0: [sdi] Write cache: enabled, read cache: enabled, supports DPO and FUA > [ 15.552773] sd 2:0:0:0: [sdi] Preferred minimum I/O size 512 bytes > [ 15.552776] sd 2:0:0:0: [sdi] Optimal transfer size 524288 bytes > [ 15.575373] sd 2:0:0:0: [sdi] tag#62 Done: SUCCESS Result: hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s > [ 15.575377] sd 2:0:0:0: [sdi] tag#62 CDB: Inquiry 12 01 b9 00 04 00 > [ 15.575380] sd 2:0:0:0: [sdi] tag#62 Sense Key : Illegal Request [current] > [ 15.575383] sd 2:0:0:0: [sdi] tag#62 Add. Sense: Invalid field in cdb > [ 15.645749] sd 2:0:0:0: [sdi] Attached SCSI disk > > But we don't even see the `sg_alloc: dev=...` message that is logged in > `sg_alloc()`. And between the change above and the call to `sg_alloc()`, > there is only the character device allocation; and if that failed, it > would print an error. So either the bind is never even tried, or the new > `blk_get_queue()` fails to get a reference. > Which is odd, since the only way that would happen is, if the queue > was marked dying; but we see that the stack is using it for LUN probing > in `sd`. Yes, if scsi_device_get() works fine, but blk_get_queue() has problems, it seems to me that sg_add_device() can be called with scsi_device queue mark dying? This is odd, but I'm not sure if it's the case. Thanks, Kuai > >> This change (bisected) triggers a regression in our KVM on s390x CI. The >> symptom is that a “scsi_debug device” does not bind to the scsi_generic >> driver. On s390x you can reproduce the problem as follows (I have not >> tested on x86): >> >> With this patch applied: >> >> $ sudo modprobe scsi_debug > > One more thing maybe worth mentioning: in the kernel configuration we > use in the CI we have `sg` built-in. I guess most have it built as > module. > >> $ # Get the 'scsi_host,channel,target_number,LUN' tuple for the scsi_debug device >> $ lsscsi |grep scsi_debug |awk '{ print $1 }' >> [0:0:0:0] >> $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic >> stat: cannot statx '/sys/bus/scsi/devices/0:0:0:0/scsi_generic': No such file or directory >> >> >> Patch reverted: >> >> $ sudo modprobe scsi_debug >> $ lsscsi |grep scsi_debug |awk '{ print $1 }' >> [0:0:0:0] >> $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic >> File: /sys/bus/scsi/devices/0:0:0:0/scsi_generic >> Size: 0 Blocks: 0 IO Block: 4096 directory >> Device: 0,20 Inode: 12155 Links: 3 >> … > > That's all I got from looking at it earlier, so far. >
Hi, 在 2023/07/05 10:16, Yu Kuai 写道: > Hi, > > 在 2023/07/05 2:51, Benjamin Block 写道: >> On Tue, Jul 04, 2023 at 07:04:00PM +0200, Marc Hartmayer wrote: >>> On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai >>> <yukuai1@huaweicloud.com> wrote: >>>> From: Yu Kuai <yukuai3@huawei.com> >>>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >>>> index 2433eeef042a..dcb73787c29d 100644 >>>> --- a/drivers/scsi/sg.c >>>> +++ b/drivers/scsi/sg.c >>>> @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) >>>> int error; >>>> unsigned long iflags; >>>> - error = scsi_device_get(scsidp); >>>> + error = blk_get_queue(scsidp->request_queue); >>>> if (error) >>>> return error; >> >> Might be interesting as well. Marc showed me a `dmesg` snipped earlier >> from when the bind fails: >> >> [ 15.441817] scsi host2: scsi_eh_2: sleeping >> [ 15.441899] scsi_debug:sdebug_driver_probe: scsi_debug: trim >> poll_queues to 0. poll_q/nr_hw = (0/1) >> [ 15.441907] scsi host2: scsi_debug: version 0191 [20210520] >> dev_size_mb=8, opts=0x0, submit_queues=1, >> statistics=0 >> [ 15.442078] scsi host2: scsi_scan_host_selected: >> <4294967295:4294967295:18446744073709551615> >> [ 15.442267] scsi 2:0:0:0: scsi scan: INQUIRY pass 1 length 36 >> [ 15.442286] scsi 2:0:0:0: scsi scan: INQUIRY successful with >> code 0x0 >> [ 15.442296] scsi 2:0:0:0: scsi scan: INQUIRY pass 2 length 96 >> [ 15.442308] scsi 2:0:0:0: scsi scan: INQUIRY successful with >> code 0x0 >> [ 15.442317] scsi 2:0:0:0: Direct-Access Linux >> scsi_debug 0191 PQ: 0 ANSI: 7 >> [ 15.442554] scsi 2:0:0:0: Power-on or device reset occurred >> [ 15.442560] scsi 2:0:0:0: tag#50 Done: SUCCESS Result: >> hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s >> [ 15.442565] scsi 2:0:0:0: tag#50 CDB: Report supported operation >> codes a3 0c 01 88 00 00 00 00 00 14 00 00 >> [ 15.442569] scsi 2:0:0:0: tag#50 Sense Key : Unit Attention >> [current] >> [ 15.442573] scsi 2:0:0:0: tag#50 Add. Sense: Power on occurred >> >> The bind should happend around here somewhere I think. >> >> [ 15.472680] sd 2:0:0:0: scsi scan: Sending REPORT LUNS to (try 0) >> [ 15.472703] sd 2:0:0:0: scsi scan: REPORT LUNS successful (try >> 0) result 0x0 >> [ 15.472706] sd 2:0:0:0: scsi scan: REPORT LUN scan >> [ 15.472709] sd 2:0:0:0: scsi scan: device exists on 2:0:0:0 >> [ 15.492874] sd 2:0:0:0: [sdi] 16384 512-byte logical blocks: >> (8.39 MB/8.00 MiB) >> [ 15.502853] sd 2:0:0:0: [sdi] Write Protect is off >> [ 15.502856] sd 2:0:0:0: [sdi] Mode Sense: 73 00 10 08 >> [ 15.522819] sd 2:0:0:0: [sdi] Write cache: enabled, read cache: >> enabled, supports DPO and FUA >> [ 15.552773] sd 2:0:0:0: [sdi] Preferred minimum I/O size 512 bytes >> [ 15.552776] sd 2:0:0:0: [sdi] Optimal transfer size 524288 bytes >> [ 15.575373] sd 2:0:0:0: [sdi] tag#62 Done: SUCCESS Result: >> hostbyte=DID_OK driverbyte=DRIVER_OK cmd_age=0s >> [ 15.575377] sd 2:0:0:0: [sdi] tag#62 CDB: Inquiry 12 01 b9 00 04 00 >> [ 15.575380] sd 2:0:0:0: [sdi] tag#62 Sense Key : Illegal Request >> [current] >> [ 15.575383] sd 2:0:0:0: [sdi] tag#62 Add. Sense: Invalid field >> in cdb >> [ 15.645749] sd 2:0:0:0: [sdi] Attached SCSI disk >> >> But we don't even see the `sg_alloc: dev=...` message that is logged in >> `sg_alloc()`. And between the change above and the call to `sg_alloc()`, >> there is only the character device allocation; and if that failed, it >> would print an error. So either the bind is never even tried, or the new >> `blk_get_queue()` fails to get a reference. >> Which is odd, since the only way that would happen is, if the queue >> was marked dying; but we see that the stack is using it for LUN probing >> in `sd`. > > Yes, if scsi_device_get() works fine, but blk_get_queue() has problems, > it seems to me that sg_add_device() can be called with scsi_device queue > mark dying? This is odd, but I'm not sure if it's the case. Sorry that I totally messed how blk_get_queue() is called, it returns true on success and false on failure, not 0 on success and errno on failure. Sorry for all the troble. Kuai > > Thanks, > Kuai >> >>> This change (bisected) triggers a regression in our KVM on s390x CI. The >>> symptom is that a “scsi_debug device” does not bind to the scsi_generic >>> driver. On s390x you can reproduce the problem as follows (I have not >>> tested on x86): >>> >>> With this patch applied: >>> >>> $ sudo modprobe scsi_debug >> >> One more thing maybe worth mentioning: in the kernel configuration we >> use in the CI we have `sg` built-in. I guess most have it built as >> module. >> >>> $ # Get the 'scsi_host,channel,target_number,LUN' tuple for the >>> scsi_debug device >>> $ lsscsi |grep scsi_debug |awk '{ print $1 }' >>> [0:0:0:0] >>> $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic >>> stat: cannot statx '/sys/bus/scsi/devices/0:0:0:0/scsi_generic': No >>> such file or directory >>> >>> >>> Patch reverted: >>> >>> $ sudo modprobe scsi_debug >>> $ lsscsi |grep scsi_debug |awk '{ print $1 }' >>> [0:0:0:0] >>> $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic >>> File: /sys/bus/scsi/devices/0:0:0:0/scsi_generic >>> Size: 0 Blocks: 0 IO Block: 4096 directory >>> Device: 0,20 Inode: 12155 Links: 3 >>> … >> >> That's all I got from looking at it earlier, so far. >> > > . >
Hi, 在 2023/07/05 9:43, Yu Kuai 写道: > Hi, > > 在 2023/07/05 1:04, Marc Hartmayer 写道: >> On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai >> <yukuai1@huaweicloud.com> wrote: >>> From: Yu Kuai <yukuai3@huawei.com> >>> >>> In order to prevent request_queue to be freed before cleaning up >>> blktrace debugfs entries, commit db59133e9279 ("scsi: sg: fix blktrace >>> debugfs entries leakage") use scsi_device_get(), however, >>> scsi_device_get() will also grab scsi module reference and scsi module >>> can't be removed. >>> >>> It's reported that blktests can't unload scsi_debug after block/001: >>> >>> blktests (master) # ./check block >>> block/001 (stress device hotplugging) [failed] >>> +++ /root/blktests/results/nodev/block/001.out.bad 2023-06-19 >>> Running block/001 >>> Stressing sd >>> +modprobe: FATAL: Module scsi_debug is in use. >>> >>> Fix this problem by grabbing request_queue reference directly, so that >>> scsi host module can still be unloaded while request_queue will be >>> pinged by sg device. >>> >>> Reported-by: Chaitanya Kulkarni <chaitanyak@nvidia.com> >>> Link: >>> https://lore.kernel.org/all/1760da91-876d-fc9c-ab51-999a6f66ad50@nvidia.com/ >>> >>> Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage") >>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>> --- >>> drivers/scsi/sg.c | 6 +++--- >>> 1 file changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >>> index 2433eeef042a..dcb73787c29d 100644 >>> --- a/drivers/scsi/sg.c >>> +++ b/drivers/scsi/sg.c >>> @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) >>> int error; >>> unsigned long iflags; >>> - error = scsi_device_get(scsidp); >>> + error = blk_get_queue(scsidp->request_queue); >>> if (error) >>> return error; >>> @@ -1558,7 +1558,7 @@ sg_add_device(struct device *cl_dev) >>> out: >>> if (cdev) >>> cdev_del(cdev); >>> - scsi_device_put(scsidp); >>> + blk_put_queue(scsidp->request_queue); >>> return error; >>> } >>> @@ -1575,7 +1575,7 @@ sg_device_destroy(struct kref *kref) >>> */ >>> blk_trace_remove(q); >>> - scsi_device_put(sdp->device); >>> + blk_put_queue(q); >>> write_lock_irqsave(&sg_index_lock, flags); >>> idr_remove(&sg_index_idr, sdp->index); >>> -- >>> 2.39.2 >> >> Hi, >> >> This change (bisected) triggers a regression in our KVM on s390x CI. The >> symptom is that a “scsi_debug device” does not bind to the scsi_generic >> driver. On s390x you can reproduce the problem as follows (I have not >> tested on x86): >> >> With this patch applied: >> >> $ sudo modprobe scsi_debug >> $ # Get the 'scsi_host,channel,target_number,LUN' tuple for the >> scsi_debug device >> $ lsscsi |grep scsi_debug |awk '{ print $1 }' >> [0:0:0:0] >> $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic >> stat: cannot statx '/sys/bus/scsi/devices/0:0:0:0/scsi_generic': No >> such file or directory >> >> >> Patch reverted: >> > > I didn't figure out the root cause, howver, have you tried to reviert > this patch as well? > > db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage" Never mind this, root cause is that the checking of return value of blk_get_queue() is wrong. This shoud be fixed by following patch: diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index 89fa046c7158..0d8afffd1683 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -1497,9 +1497,10 @@ sg_add_device(struct device *cl_dev) int error; unsigned long iflags; - error = blk_get_queue(scsidp->request_queue); - if (error) - return error; + if (!blk_get_queue(scsidp->request_queue)) { + pr_warn("%s: get scsi_device queue failed\n", __func__); + return -ENODEV; + } error = -ENOMEM; cdev = cdev_alloc(); > > Thanks, > Kuai >> $ sudo modprobe scsi_debug >> $ lsscsi |grep scsi_debug |awk '{ print $1 }' >> [0:0:0:0] >> $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic >> File: /sys/bus/scsi/devices/0:0:0:0/scsi_generic >> Size: 0 Blocks: 0 IO Block: 4096 directory >> Device: 0,20 Inode: 12155 Links: 3 >> … >> >> Any ideas? >> >> Marc >> . >> > > . >
On Wed, Jul 05, 2023 at 10:28 AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > Hi, > > 在 2023/07/05 9:43, Yu Kuai 写道: >> Hi, >> >> 在 2023/07/05 1:04, Marc Hartmayer 写道: >>> On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai >>> <yukuai1@huaweicloud.com> wrote: >>>> From: Yu Kuai <yukuai3@huawei.com> >>>> >>>> In order to prevent request_queue to be freed before cleaning up >>>> blktrace debugfs entries, commit db59133e9279 ("scsi: sg: fix blktrace >>>> debugfs entries leakage") use scsi_device_get(), however, >>>> scsi_device_get() will also grab scsi module reference and scsi module >>>> can't be removed. >>>> >>>> It's reported that blktests can't unload scsi_debug after block/001: >>>> >>>> blktests (master) # ./check block >>>> block/001 (stress device hotplugging) [failed] >>>> +++ /root/blktests/results/nodev/block/001.out.bad 2023-06-19 >>>> Running block/001 >>>> Stressing sd >>>> +modprobe: FATAL: Module scsi_debug is in use. >>>> >>>> Fix this problem by grabbing request_queue reference directly, so that >>>> scsi host module can still be unloaded while request_queue will be >>>> pinged by sg device. >>>> >>>> Reported-by: Chaitanya Kulkarni <chaitanyak@nvidia.com> >>>> Link: >>>> https://lore.kernel.org/all/1760da91-876d-fc9c-ab51-999a6f66ad50@nvidia.com/ >>>> >>>> Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage") >>>> Signed-off-by: Yu Kuai <yukuai3@huawei.com> >>>> --- >>>> drivers/scsi/sg.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c >>>> index 2433eeef042a..dcb73787c29d 100644 >>>> --- a/drivers/scsi/sg.c >>>> +++ b/drivers/scsi/sg.c >>>> @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) >>>> int error; >>>> unsigned long iflags; >>>> - error = scsi_device_get(scsidp); >>>> + error = blk_get_queue(scsidp->request_queue); >>>> if (error) >>>> return error; >>>> @@ -1558,7 +1558,7 @@ sg_add_device(struct device *cl_dev) >>>> out: >>>> if (cdev) >>>> cdev_del(cdev); >>>> - scsi_device_put(scsidp); >>>> + blk_put_queue(scsidp->request_queue); >>>> return error; >>>> } >>>> @@ -1575,7 +1575,7 @@ sg_device_destroy(struct kref *kref) >>>> */ >>>> blk_trace_remove(q); >>>> - scsi_device_put(sdp->device); >>>> + blk_put_queue(q); >>>> write_lock_irqsave(&sg_index_lock, flags); >>>> idr_remove(&sg_index_idr, sdp->index); >>>> -- >>>> 2.39.2 >>> >>> Hi, >>> >>> This change (bisected) triggers a regression in our KVM on s390x CI. The >>> symptom is that a “scsi_debug device” does not bind to the scsi_generic >>> driver. On s390x you can reproduce the problem as follows (I have not >>> tested on x86): >>> >>> With this patch applied: >>> >>> $ sudo modprobe scsi_debug >>> $ # Get the 'scsi_host,channel,target_number,LUN' tuple for the >>> scsi_debug device >>> $ lsscsi |grep scsi_debug |awk '{ print $1 }' >>> [0:0:0:0] >>> $ sudo stat /sys/bus/scsi/devices/0:0:0:0/scsi_generic >>> stat: cannot statx '/sys/bus/scsi/devices/0:0:0:0/scsi_generic': No >>> such file or directory >>> >>> >>> Patch reverted: >>> >> >> I didn't figure out the root cause, howver, have you tried to reviert >> this patch as well? >> >> db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage" > > Never mind this, root cause is that the checking of return value of > blk_get_queue() is wrong. > > This shoud be fixed by following patch: > > diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c > index 89fa046c7158..0d8afffd1683 100644 > --- a/drivers/scsi/sg.c > +++ b/drivers/scsi/sg.c > @@ -1497,9 +1497,10 @@ sg_add_device(struct device *cl_dev) > int error; > unsigned long iflags; > > - error = blk_get_queue(scsidp->request_queue); > - if (error) > - return error; > + if (!blk_get_queue(scsidp->request_queue)) { > + pr_warn("%s: get scsi_device queue failed\n", __func__); > + return -ENODEV; > + } Hi Kuai, I just tried your fix and it works - thanks. Tested-by: Marc Hartmayer <mhartmay@linux.ibm.com> Marc […snip]
diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c index 2433eeef042a..dcb73787c29d 100644 --- a/drivers/scsi/sg.c +++ b/drivers/scsi/sg.c @@ -1497,7 +1497,7 @@ sg_add_device(struct device *cl_dev) int error; unsigned long iflags; - error = scsi_device_get(scsidp); + error = blk_get_queue(scsidp->request_queue); if (error) return error; @@ -1558,7 +1558,7 @@ sg_add_device(struct device *cl_dev) out: if (cdev) cdev_del(cdev); - scsi_device_put(scsidp); + blk_put_queue(scsidp->request_queue); return error; } @@ -1575,7 +1575,7 @@ sg_device_destroy(struct kref *kref) */ blk_trace_remove(q); - scsi_device_put(sdp->device); + blk_put_queue(q); write_lock_irqsave(&sg_index_lock, flags); idr_remove(&sg_index_idr, sdp->index);