SCSI: Fix some locking issues

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.

Quoted 8 lines · Elias Oltmanns

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?

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.

Quoted 3 lines

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.

Quoted 7 lines · Elias Oltmanns

Quoted 3 lines

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.

Yep, blk_plug_device() needs to be called with the queue lock held.

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Elias Oltmanns

Quoted 3 lines

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.

Quoted 1 line

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Quoted 10 lines · James Bottomley

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Elias Oltmanns

Quoted 3 lines

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.

Quoted 5 lines

Quoted 1 line

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Quoted 12 lines · Jens Axboe

Quoted 10 lines · James Bottomley

Quoted 9 lines · Jens Axboe

Quoted 7 lines · Elias Oltmanns

Quoted 3 lines

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.

Quoted 8 lines

Quoted 5 lines

Quoted 1 line

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Not necessarily ... only for QUEUE_FLAG_CLUSTER. That's really only in this one place and then the one in blk_remove_plug would have to become test_and_clear_bit. All the other places barring loop_unplug() are only tests (which don't affect the atomicity).

It's just for SCSI the double spin lock followed by double spin unlock to get the locking right is kind of nasty ... I'm just wondering what the universe would look like if it were rendered unnecessary.

Quoted 19 lines · James Bottomley

Quoted 11 lines · Jens Axboe

Quoted 6 lines · James Bottomley

Quoted 1 line · Jens Axboe

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Not necessarily ... only for QUEUE_FLAG_CLUSTER. That's really only in this one place and then the one in blk_remove_plug would have to become test_and_clear_bit. All the other places barring loop_unplug() are only tests (which don't affect the atomicity).

Quoted 3 lines

It's just for SCSI the double spin lock followed by double spin unlock to get the locking right is kind of nasty ... I'm just wondering what the universe would look like if it were rendered unnecessary.

We have to consider one more thing: Without the locking in blk_plug_device(), the following sequence of events may occur:

Quoted 20 lines · Elias Oltmanns

Quoted 19 lines · James Bottomley

Quoted 11 lines · Jens Axboe

Quoted 6 lines · James Bottomley

Quoted 1 line · Jens Axboe

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Not necessarily ... only for QUEUE_FLAG_CLUSTER. That's really only in this one place and then the one in blk_remove_plug would have to become test_and_clear_bit. All the other places barring loop_unplug() are only tests (which don't affect the atomicity).

Quoted 6 lines

Quoted 3 lines

It's just for SCSI the double spin lock followed by double spin unlock to get the locking right is kind of nasty ... I'm just wondering what the universe would look like if it were rendered unnecessary.

We have to consider one more thing: Without the locking in blk_plug_device(), the following sequence of events may occur:

Actually, it's worse than that. Locking is required in order to make absolutely sure that the unplug_timer is active iff QUEUE_FLAG_PLUGGED is set. Admittedly, it seems *very* unlikely that blk_remove_plug() will complete before the call to mod_timer() in blk_plug_device() even though it has started only *after* a call to test_and_set_bit(). However, if such a thing would ever happen, it could have dire consequences.

Quoted 28 lines · Elias Oltmanns

Quoted 20 lines · Elias Oltmanns

Quoted 19 lines · James Bottomley

Quoted 11 lines · Jens Axboe

Quoted 6 lines · James Bottomley

Quoted 1 line · Jens Axboe

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Not necessarily ... only for QUEUE_FLAG_CLUSTER. That's really only in this one place and then the one in blk_remove_plug would have to become test_and_clear_bit. All the other places barring loop_unplug() are only tests (which don't affect the atomicity).

Quoted 6 lines

Quoted 3 lines

It's just for SCSI the double spin lock followed by double spin unlock to get the locking right is kind of nasty ... I'm just wondering what the universe would look like if it were rendered unnecessary.

We have to consider one more thing: Without the locking in blk_plug_device(), the following sequence of events may occur:

Quoted 6 lines

Actually, it's worse than that. Locking is required in order to make absolutely sure that the unplug_timer is active iff QUEUE_FLAG_PLUGGED is set. Admittedly, it seems *very* unlikely that blk_remove_plug() will complete before the call to mod_timer() in blk_plug_device() even though it has started only *after* a call to test_and_set_bit(). However, if such a thing would ever happen, it could have dire consequences.

Both are races possible without either atomic bitops or the queue lock being held. We can't properly mix eg set_bit() and __set_bit(). The plugged bit is the most hammered, so it's staying non-atomic and SCSI will need to provide proper locking there.

Quoted 36 lines · Jens Axboe

Quoted 28 lines · Elias Oltmanns

Quoted 20 lines · Elias Oltmanns

Quoted 19 lines · James Bottomley

Quoted 11 lines · Jens Axboe

Quoted 6 lines · James Bottomley

Quoted 1 line · Jens Axboe

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Not necessarily ... only for QUEUE_FLAG_CLUSTER. That's really only in this one place and then the one in blk_remove_plug would have to become test_and_clear_bit. All the other places barring loop_unplug() are only tests (which don't affect the atomicity).

Quoted 6 lines

Quoted 3 lines

It's just for SCSI the double spin lock followed by double spin unlock to get the locking right is kind of nasty ... I'm just wondering what the universe would look like if it were rendered unnecessary.

We have to consider one more thing: Without the locking in blk_plug_device(), the following sequence of events may occur:

Quoted 6 lines

Actually, it's worse than that. Locking is required in order to make absolutely sure that the unplug_timer is active iff QUEUE_FLAG_PLUGGED is set. Admittedly, it seems *very* unlikely that blk_remove_plug() will complete before the call to mod_timer() in blk_plug_device() even though it has started only *after* a call to test_and_set_bit(). However, if such a thing would ever happen, it could have dire consequences.

Quoted 4 lines

Both are races possible without either atomic bitops or the queue lock being held. We can't properly mix eg set_bit() and __set_bit(). The plugged bit is the most hammered, so it's staying non-atomic and SCSI will need to provide proper locking there.

You're the boss.

Actually, after all of this, it looks like the host queue plug is superfluous. If the host actually says not ready from scsi_host_queue_ready() we go to the not ready processing clause in scsi_prep_fn() which actually checks the outstanding on the current device and plugs the queue if there aren't any commands. This is actually more correct behaviour than a blind plug regardless (and it's also done under the queue lock), so I think this is the correct fix.

Quoted 37 lines · Jens Axboe

Quoted 28 lines · Elias Oltmanns

Quoted 20 lines · Elias Oltmanns

Quoted 19 lines · James Bottomley

Quoted 11 lines · Jens Axboe

Quoted 6 lines · James Bottomley

Quoted 1 line · Jens Axboe

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Not necessarily ... only for QUEUE_FLAG_CLUSTER. That's really only in this one place and then the one in blk_remove_plug would have to become test_and_clear_bit. All the other places barring loop_unplug() are only tests (which don't affect the atomicity).

Quoted 6 lines

Quoted 3 lines

It's just for SCSI the double spin lock followed by double spin unlock to get the locking right is kind of nasty ... I'm just wondering what the universe would look like if it were rendered unnecessary.

We have to consider one more thing: Without the locking in blk_plug_device(), the following sequence of events may occur:

Quoted 6 lines

Actually, it's worse than that. Locking is required in order to make absolutely sure that the unplug_timer is active iff QUEUE_FLAG_PLUGGED is set. Admittedly, it seems *very* unlikely that blk_remove_plug() will complete before the call to mod_timer() in blk_plug_device() even though it has started only *after* a call to test_and_set_bit(). However, if such a thing would ever happen, it could have dire consequences.

Quoted 4 lines

Both are races possible without either atomic bitops or the queue lock being held. We can't properly mix eg set_bit() and __set_bit(). The plugged bit is the most hammered, so it's staying non-atomic and SCSI will need to provide proper locking there.

You're the boss.

Actually, after all of this, it looks like the host queue plug is superfluous. If the host actually says not ready from scsi_host_queue_ready() we go to the not ready processing clause in scsi_prep_fn() which actually checks the outstanding on the current device and plugs the queue if there aren't any commands. This is actually more correct behaviour than a blind plug regardless (and it's also done under the queue lock), so I think this is the correct fix.

Quoted 45 lines · James Bottomley

Quoted 37 lines · Jens Axboe

Quoted 28 lines · Elias Oltmanns

Quoted 20 lines · Elias Oltmanns

Quoted 19 lines · James Bottomley

Quoted 11 lines · Jens Axboe

Quoted 6 lines · James Bottomley

Quoted 1 line · Jens Axboe

Yep, blk_plug_device() needs to be called with the queue lock held.

That's what the comment says ... but if you replaced the test_bit with an atomic operation then the rest of it does look to be in no need of serialisation ... unless there's something I missed?

Indeed, but then you would have to use atomic bitops everywhere and that is the bit we moved away from.

Not necessarily ... only for QUEUE_FLAG_CLUSTER. That's really only in this one place and then the one in blk_remove_plug would have to become test_and_clear_bit. All the other places barring loop_unplug() are only tests (which don't affect the atomicity).

Quoted 6 lines

Quoted 3 lines

It's just for SCSI the double spin lock followed by double spin unlock to get the locking right is kind of nasty ... I'm just wondering what the universe would look like if it were rendered unnecessary.

We have to consider one more thing: Without the locking in blk_plug_device(), the following sequence of events may occur:

Quoted 6 lines

Actually, it's worse than that. Locking is required in order to make absolutely sure that the unplug_timer is active iff QUEUE_FLAG_PLUGGED is set. Admittedly, it seems *very* unlikely that blk_remove_plug() will complete before the call to mod_timer() in blk_plug_device() even though it has started only *after* a call to test_and_set_bit(). However, if such a thing would ever happen, it could have dire consequences.

Quoted 4 lines

Both are races possible without either atomic bitops or the queue lock being held. We can't properly mix eg set_bit() and __set_bit(). The plugged bit is the most hammered, so it's staying non-atomic and SCSI will need to provide proper locking there.

You're the boss.

Quoted 7 lines

Actually, after all of this, it looks like the host queue plug is superfluous. If the host actually says not ready from scsi_host_queue_ready() we go to the not ready processing clause in scsi_prep_fn() which actually checks the outstanding on the current device and plugs the queue if there aren't any commands. This is actually more correct behaviour than a blind plug regardless (and it's also done under the queue lock), so I think this is the correct fix.

Indeed, a very neat way out. Will this be queued up for stable too?

Quoted 10 lines · James Bottomley

Quoted 8 lines · Elias Oltmanns

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.

Quoted 3 lines

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.

Quoted 3 lines

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.

Quoted 16 lines · Elias Oltmanns

Quoted 13 lines · James Bottomley

Quoted 8 lines · Elias Oltmanns

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?

Quoted 4 lines

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?

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

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

Quoted 5 lines

Quoted 4 lines

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.

Quoted 3 lines

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.

Quoted 2 lines

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?

Quoted 9 lines · Elias Oltmanns

Quoted 3 lines

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?

The assumption we make (and it is believed to be true on all SMP systems) is that a write to a naturally aligned memory location that is sized <= sizeof(long) is atomic. That is, a reader will get either the previous value or the subsequent value, not a mixture. The RCU code relies heavily on this assumption.

Quoted 11 lines · Matthew Wilcox

Quoted 9 lines · Elias Oltmanns

Quoted 3 lines

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?

Quoted 5 lines

The assumption we make (and it is believed to be true on all SMP systems) is that a write to a naturally aligned memory location that is sized <= sizeof(long) is atomic. That is, a reader will get either the previous value or the subsequent value, not a mixture. The RCU code relies heavily on this assumption.

Does that mean that where ever I have

spin_lock_irqsave(some_lock, flags);
var = some_val;
spin_unlock_irqrestore(some_lock, flags);

I could just as well discard the locking provided that sizeof(var) <= sizeof(long)
because the assignment of some_val to var will be atomic anyway?

Quoted 1 line · Elias Oltmanns

Matthew Wilcox <[email protected]> wrote:

Quoted 8 lines

Quoted 5 lines

The assumption we make (and it is believed to be true on all SMP systems) is that a write to a naturally aligned memory location that is sized <= sizeof(long) is atomic. That is, a reader will get either the previous value or the subsequent value, not a mixture. The RCU code relies heavily on this assumption.

Does that mean that where ever I have

Quoted 7 lines

spin_lock_irqsave(some_lock, flags);
var = some_val;
spin_unlock_irqrestore(some_lock, flags);

I could just as well discard the locking provided that sizeof(var) <= sizeof(long)
because the assignment of some_val to var will be atomic anyway?

I think you're still confused about the difference between integral observation of values and serialisation.

Assignment is always integral. In your example above anything observing the variable always sees either the previous value or the one you set it to, nothing else. So, if all you're doing is setting a flag and clearing it (as in setting it to an integral value, not setting it via a bit operation) and checking it, no locking is needed. It's when you start doing operations on variables that require serialisation, or the variables themselves are absolute indicators of events that need serialisation that you start having to introduce locking.

Quoted 5 lines · Matthew Wilcox

The assumption we make (and it is believed to be true on all SMP systems) is that a write to a naturally aligned memory location that is sized <= sizeof(long) is atomic. That is, a reader will get either the previous value or the subsequent value, not a mixture. The RCU code relies heavily on this assumption.

Does that mean that where ever I have

spin_lock_irqsave(some_lock, flags);
var = some_val;
spin_unlock_irqrestore(some_lock, flags);

I could just as well discard the locking provided that sizeof(var) <= sizeof(long)
because the assignment of some_val to var will be atomic anyway?

Quoted 17 lines · James Bottomley

Quoted 16 lines · Elias Oltmanns

Quoted 13 lines · James Bottomley

Quoted 8 lines · Elias Oltmanns

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?

Quoted 6 lines

Quoted 4 lines

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.

Quoted 1 line

Quoted 2 lines

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?

Quoted 2 lines

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.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

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.

Quoted 4 lines

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).

Quoted 1 line · Elias Oltmanns

James Bottomley <[email protected]> wrote:

Quoted 8 lines

Quoted 2 lines

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.

Look more closely at the requirements for the decrements: There have to be no outstanding commands: nothing can be in scsi_finish_command for the device (or the host for host_blocked).