Re: [PATCH] scsi: core: Call blk_mq_free_tag_set() earlier

[Ming Lei <ming.lei@redhat.com>]

On Thu, Jun 30, 2022 at 09:01:39AM +0800, Ming Lei wrote:
> Hi Bart,
> 
> I'd rather to understand the issue first.
> 
> On Wed, Jun 29, 2022 at 02:49:27PM -0700, Bart Van Assche wrote:
> > On 6/28/22 18:17, Ming Lei wrote:
> > > On Tue, Jun 28, 2022 at 10:56:12AM -0700, Bart Van Assche wrote:
> > > > There are two .exit_cmd_priv implementations. Both implementations use the
> > > > SCSI host pointer. Make sure that the SCSI host pointer is valid when
> > > > .exit_cmd_priv is called by moving the .exit_cmd_priv calls from
> > > > scsi_device_dev_release() to scsi_forget_host(). Moving
> 
> .exit_cmd_priv is actually called from scsi_host_dev_release() instead
> of scsi_device_dev_release(). Both scsi host pointer and host->shost_data is
> still valid when calling .exit_cmd_priv via scsi_mq_destroy_tags().
> 
> Previously I fixed[1] one similar issue, and that is caused by early module
> unloading, and anywhere host->hostt is referred, the scsi driver module
> should be prevented from being unloaded.
> 
> 
> [1] f2b85040acec scsi: core: Put LLD module refcnt after SCSI device is released

Hi Bart,

BTW, Changhui reported one very similar issue when running elevator
switch/scsi debug LUN hotplug.

From Changhui's report, the issue is basically same with what
f2b85040acec tried to address, but the try_module_get() in
scsi_device_dev_release() may fail, so the scsi_debug module
still can be unloaded.

The thing is that sdev can be released in async style, and target/host
release is triggered by scsi_device_dev_release_usercontext().

So after scsi_host_remove() returns, the shost may still be live from
driver core/sysfs viewpoint, and its release handler can be called
after the LLD module is unloaded. Then this kind of issue is triggered.

Seems there are at least two approaches for fixing the issue:

1) the one suggested in this thread:
- moving any reference to shost->hostt in host release handler into
scsi_host_remove(), and scsi_mq_destroy_tags()/scsi_proc_hostdir_rm(shost->hostt)()
should be covered at least

2) wait until all targets are released in scsi_host_remove()

I am fine with either of the two approaches.

Bart, please let me know if you are working towards the approach in 1).
If not, I have one patch which implements 2).

BTW, after either 1) or 2) is done, commit f2b85040acec can be reverted.


Thanks,
Ming