Re: [PATCH 07/10] block: reorganize throtl_get_tg() andblk_throtl_bio()

[Vivek Goyal]

On Wed, Oct 19, 2011 at 10:06:25AM -0700, Tejun Heo wrote:
> Hello,
> 
> On Wed, Oct 19, 2011 at 10:56:22AM -0400, Vivek Goyal wrote:
> > A driver could call blk_cleanup_queue(), mark the queue DEAD and then
> > free the driver provided spin lock. So once queue is DEAD one could
> > not rely on queue lock still being there. That's the reason I did
> > not try to take queue lock again if queue is marked DEAD.
> >
> > Now I see the change that blk_cleanup_queue will start poiting to
> > internal queue lock (Thought it is racy). This will atleast make
> > sure that some spinlock is around. So now this change should be
> > fine.
> 
> The problem with the current code is that all those are not properly
> synchronized.  Drivers shouldn't destroy lock or any other stuff until
> blk_cleanup_queue() is complete and once queue cleanup is done block
> layer shouldn't call out to driver.

If queue lock is provided by driver then block layer has no choice but
to call into driver even after cleanup(). (As after shutdown(), till
release() is called, you will need spin lock to check whether queue is
dead or not).
 
> 
> Currently, the code has different opportunistic checks which can catch
> most of those cases but unfortunatly I think it just makes the bugs
> more obscure.
> 
> That said, we probably should be switching to internal lock once
> clenaup is complete.

So even switching to internal lock is racy. Christoph suggeted to break down
this sharing of queue lock and driver lock and suggested always use
internal queue lock and modify drivers to use their own lock and manage it.
It makes sense though it might be lot of work to fix drivers.

Thanks
Vivek
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/