Re: [PATCH] Revert "module, async: async_synchronize_full() on module init iff async is used"

[Tejun Heo <tj@kernel.org>]

On Thu, Jan 27, 2022 at 03:39:53PM -0800, Igor Pylypiv wrote:
> This reverts commit 774a1221e862b343388347bac9b318767336b20b.
> 
> We need to finish all async code before the module init sequence is done.
> In the reverted commit the PF_USED_ASYNC flag was added to mark a thread
> that called async_schedule(). Then the PF_USED_ASYNC flag was used to
> determine whether or not async_synchronize_full() needs to be invoked.
> This works when modprobe thread is calling async_schedule(), but it
> does not work if module dispatches init code to a worker thread which
> then calls async_schedule().
> 
> For example, PCI driver probing is invoked from a worker thread based on
> a node where device is attached:
> 
> 	if (cpu < nr_cpu_ids)
> 		error = work_on_cpu(cpu, local_pci_probe, &ddi);
> 	else
> 		error = local_pci_probe(&ddi);
> 
> We end up in a situation where a worker thread gets the PF_USED_ASYNC flag
> set instead of the modprobe thread. As a result, async_synchronize_full()
> is not invoked and modprobe completes without waiting for the async code
> to finish.
> 
> The issue was discovered while loading the pm80xx driver:
> (scsi_mod.scan=async)
> 
> modprobe pm80xx                      worker
> ...
>   do_init_module()
>   ...
>     pci_call_probe()
>       work_on_cpu(local_pci_probe)
>                                      local_pci_probe()
>                                        pm8001_pci_probe()
>                                          scsi_scan_host()
>                                            async_schedule()
>                                            worker->flags |= PF_USED_ASYNC;
>                                      ...
>       < return from worker >
>   ...
>   if (current->flags & PF_USED_ASYNC) <--- false
>   	async_synchronize_full();
> 
> Commit 21c3c5d28007 ("block: don't request module during elevator init")
> fixed the deadlock issue which the reverted commit 774a1221e862 ("module,
> async: async_synchronize_full() on module init iff async is used") tried
> to fix.
> 
> Since commit 0fdff3ec6d87 ("async, kmod: warn on synchronous
> request_module() from async workers") synchronous module loading
> from async is not allowed.
> 
> Given that the original deadlock issue is fixed and it is no longer allowed
> to call synchronous request_module() from async we can remove PF_USED_ASYNC
> flag to make module init consistently invoke async_synchronize_full()
> unless async module probe is requested.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> Reviewed-by: Changyuan Lyu <changyuanl@google.com>

That's quite a walk down the memory lane and I agree with your analysis. The
PF_USED_ASYNC is redundant for correctness with the removal of synchrnous
loading from iosched path and the WARN_ON guarantees that nothing in kernel
is creating a similar situation.

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun