Re: [PATCH] SCSI: Fix some locking issues

[Elias Oltmanns <eo@nebensachen.de>]

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
> On Wed, 2008-07-02 at 09:08 +0200, Elias Oltmanns wrote:
>> James Bottomley <James.Bottomley@HansenPartnership.com> wrote:
>
>> > On Tue, 2008-07-01 at 23:37 +0200, Elias Oltmanns wrote:
>> >> Hi James,
>> >
>> >> 
>> >> sorry for bothering you but I've just noticed that the patch below has
>> >> neither been scheduled for the stable review, nor queued up for Linus.
>> >> May be you just don't consider this serious enough for these trees but I
>> >> wanted to make sure that the situation will be dealt with eventualy. The
>> >> patch applies to 2.6.26-rc8.
>> >
>> > OK, well at first glance, the locking around device_blocked and
>> > host_blocked looks pointless.  What are the failure traces you're using
>> > to decide they need spinlock protection?
>> 
>> scsi_queue_insert() as well as scsi_finish_command() can be called at
>> any time as part of regular command completion or error handling. There
>> is no reason why the ->request_fn() for the same device or for another
>> device on the same host should not be in progress at the same time.
>
> So would I be correct in deducing you haven't seen an observed failure?

Yes, I don't even have an SMP machine.

>
> The reason no locks are necessary is that there's no race to mediate.
> The checks are only is it set or not ...

I'm not sure whether that is of any consequence. Don't get me wrong, I
really don't know and you may well be right. But how exactly does
decrementing from 2 to 1 work? Do we know for sure that there will
always be at least one bit set so reading that address will reliably
return a non zero value?

> unless we get down to zero depth in which case the decrements are done
> under lock.

Sorry, but this simply doesn't resolve the matter at hand.
scsi_finish_command() can change (host|device)_blocked values to zero at
any time currently *not* protected by any lock. In much the same way
scsi_queue_insert() can change these values from zero to something else
at any time.

>
>> > The blk_plug_queue change looks reasonable ... however, blk_plug_queue
>> > itself looks like it might not entirely need the queue lock ... I need
>> > to investigate more closely.
>> 
>> Well, I rather think it does. We have to serialise access to the
>> unplug_timer and there is a call to __set_bit() which, as I understand,
>> requires the calling function to ensure atomicity.
>
> It does at the moment ... it just looks like it could make use of
> test_and_set_bit() to avoid the requirement.  The access to the timer
> uses mod_timer() which is specifically designed not to require
> serialisation.

Concurrent calls to mod_timer() are alright; I'm not so sure what
happens when del_timer() is called at the same time (haven't checked
though, so you might be right here).

Regards,

Elias