On Thu, Apr 12, 2018 at 11:52:11AM +0000, Bart Van Assche wrote: > On Thu, 2018-04-12 at 07:34 +0200, Christoph Hellwig wrote: > > On Wed, Apr 11, 2018 at 07:58:52PM -0600, Bart Van Assche wrote: > > > Several block drivers call alloc_disk() followed by put_disk() if > > > something fails before device_add_disk() is called without calling > > > blk_cleanup_queue(). Make sure that also for this scenario a request > > > queue is dissociated from the cgroup controller. This patch avoids > > > that loading the parport_pc, paride and pf drivers triggers the > > > following kernel crash: > > > > Can we move the cleanup to put_disk in general and not just for > > this case? Having alloc/free routines pair up generally avoids > > a lot of confusion. > > Hello Christoph, > > At least the SCSI ULP drivers drop the last reference to a disk after > the blk_cleanup_queue() call. As explained in the description of commit > a063057d7c73, removing a request queue from blkcg must happen before > blk_cleanup_queue() finishes because a block driver may free the > request queue spinlock immediately after blk_cleanup_queue() returns. > So I don't think that we can move the code that removes a request > queue from blkcg into put_disk(). Another challenge is that some block > drivers (e.g. skd) clear the disk->queue pointer if device_add_disk() > has not been called to avoid that put_disk() causes a request queue > reference count imbalance. Which sounds like a very good reason not to use a driver controller lock for internals like blkcq. In fact splitting the lock used for synchronizing access to queue fields from the driver controller lock used to synchronize I/O in the legacy path in long overdue.
