From: Yu Kuai <[email protected]>
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 <[email protected]>
Link: https://lore.kernel.org/all/[email protected]/
Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage")
Signed-off-by: Yu Kuai <[email protected]>
---
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
On 6/21/23 09:01, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> 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 <[email protected]>
>
> 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 <[email protected]>
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,
--
Jens Axboe
On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai <[email protected]> wrote:
> From: Yu Kuai <[email protected]>
>
> 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 <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]/
> Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage")
> Signed-off-by: Yu Kuai <[email protected]>
> ---
> 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 <[email protected]> wrote:
> > From: Yu Kuai <[email protected]>
> > 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.
--
Best Regards, Benjamin Block / Linux on IBM Z Kernel Development
IBM Deutschland Research & Development GmbH / https://www.ibm.com/privacy
Vors. Aufs.-R.: Gregor Pillen / Geschäftsführung: David Faller
Sitz der Ges.: Böblingen / Registergericht: AmtsG Stuttgart, HRB 243294
Hi,
在 2023/07/05 1:04, Marc Hartmayer 写道:
> On Thu, Jun 22, 2023 at 12:01 AM +0800, Yu Kuai <[email protected]> wrote:
>> From: Yu Kuai <[email protected]>
>>
>> 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 <[email protected]>
>> Link: https://lore.kernel.org/all/[email protected]/
>> Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage")
>> Signed-off-by: Yu Kuai <[email protected]>
>> ---
>> 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 <[email protected]> wrote:
>>> From: Yu Kuai <[email protected]>
>>> 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
>>> <[email protected]> wrote:
>>>> From: Yu Kuai <[email protected]>
>>>> 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
>> <[email protected]> wrote:
>>> From: Yu Kuai <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Link:
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>> Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage")
>>> Signed-off-by: Yu Kuai <[email protected]>
>>> ---
>>> 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 <[email protected]> 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
>>> <[email protected]> wrote:
>>>> From: Yu Kuai <[email protected]>
>>>>
>>>> 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 <[email protected]>
>>>> Link:
>>>> https://lore.kernel.org/all/[email protected]/
>>>>
>>>> Fixes: db59133e9279 ("scsi: sg: fix blktrace debugfs entries leakage")
>>>> Signed-off-by: Yu Kuai <[email protected]>
>>>> ---
>>>> 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 <[email protected]>
Marc
[…snip]