From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756568Ab3AORhU (ORCPT ); Tue, 15 Jan 2013 12:37:20 -0500 Received: from mail-vc0-f173.google.com ([209.85.220.173]:38238 "EHLO mail-vc0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754855Ab3AORhS (ORCPT ); Tue, 15 Jan 2013 12:37:18 -0500 MIME-Version: 1.0 In-Reply-To: References: From: Linus Torvalds Date: Tue, 15 Jan 2013 09:36:57 -0800 X-Google-Sender-Auth: VM1i1V8-e6wuo-76j3o9FX9CI1o Message-ID: Subject: Re: USB device cannot be reconnected and khubd "blocked for more than 120 seconds" To: Ming Lei , Tejun Heo Cc: Alex Riesen , Alan Stern , Jens Axboe , USB list , Linux Kernel Mailing List Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org [ Added Tejun to the discussion, since he's the async go-to-guy ] On Mon, Jan 14, 2013 at 10:23 PM, Ming Lei wrote: > > But I have another idea to address the problem, and let module code call > async_synchronize_full() only if the module requires that explicitly, so how > about the below draft patch? No way. This kind of "let's just let drivers tell us when they used async helpers" is basically *asking* for buggy code. In fact, just to prove how bad it is, YOU SCREWED IT UP YOURSELF. Because it's not just sd.c that uses async_schedule(), and would need the async synchronize. It's floppy.c, it's generic scsi scanning (so scsi tapes etc), and it's libata-core.c. This kind of "let's randomly encourage people to write subtly buggy code that has magical timing dependencies, so that the developer won't likely even see it because he has fast disks etc" code is totally unacceptable. And this code was *designed* to be that kind of buggy. No, if we set a flag like this, then it needs to be set *automatically*, so that a module cannot screw this up by mistake. It could be as simple as having a per-thread flag that gets set by the __async_schedule() function, and gets cleared by fork. Then the module code could do something like /* before calling the module ->init function */ current->used_async = 0; ... if (current->used_async) async_synchronize_full(); or whatever. Tejun, comments? You can see the whole thread on lkml, but the basic problem is that the module loading doing the unconditional async_synchronize_full() has caused problems, because we have - load module A - module A does per-controller async discovery of its devices (eg scsi or ata probing) - in the async thread, it initializes somethign that needs another module B (in this case the default IO scheduler module) - modprobe for B loads the IO scheduler module successfully at the end of the module load, it does async_synchronize_full() to make sure load_module won't return before the module is ready *DEADLOCK*, because the async_synchronize_full() thing actually waits for not the module B async code (it didn't have any), but for the module *A* async code, which is waiting for module B to finish. Now, I'll happily argue that we shouldn't have this kind of "load modules from random context" behavior in the kernel, and I think the block layer is to blame for doing the IO scheduler load at an insane time. So "don't do that then" would be the best solution. Sadly, we don't even have a good way to notice that we're doing it, so "hacky workaround that at least doesn't require driver authors to care" is likely the second-best workaround. But the "hacky workaround" absolutely needs to be *automatic*. Because the "driver writers need to get this subtle untestable thing right" is *not* acceptable. That's the patch that Ming Lei did, and I refuse to have that kind of fragile crap in the kernel. Linus