Re: [PATCH RESEND 2/3 v2.6.39-rc7] block: make disk_block_events() properly wait for work cancellation

[Tejun Heo <tj@kernel.org>]

Hello, Linus.

On Tue, May 17, 2011 at 07:46:25AM -0700, Linus Torvalds wrote:
> This is pretty disgusting.
> 
> You're not using a real lock, and to compensate for that you use a
> bloccking bit-lock hack. And to make that hack extra ugly, you define
> the bit as a bitmask, and use the ilog2() macro to turn it into a bit
> pos.
> 
> Horrid. Horrid.

Heh, okay.  It's not a lock tho.  It's multiple waiter waiting for a
single event - so either explicit waitqueue or completion.  I was
doing a waitqueue but bit waitqueue didn't seem to add too much
complexity, so...

> Is there some fundamental reason why you cannot just turn the ev->lock
> into a real semaphore (allowing blocking), and then doing the dwork
> cancel under the semaphore - avoiding all the crazy bit-lock crud.

disk_check_events() can be called from non-sleepable context so we
need a spinlock protecting block count.

> Or just _add_ a semaphore to the 'struct disk_events', for chrissake.

Alright, a completion then.

> This is just too ugly to survive. And even if you fixed the ilog()
> (hint: just define the bit, and then use (1u<<BIT) to define the
> mask), it would be too ugly.

In many cases there are more C bitops than atomic or wait_bit() type
operations which take bit position rather than mask.  I find it less
painful to define constants as bit masks and using ilog2() on those
ops than doing lots of 1 << BIT.

I don't know, this is constantly painful.  More are defined as bit
masks but some prominent ones use bit positions and some even define
both.  Given that the C bitops are there by default, I wish the
explicit bitops also took bitmask and just did ilog2() internally, but
well it's too late and we're stuck with the mixed situation.

Thanks.

-- 
tejun