On Mon, 2013-06-10 at 11:23 +0200, Heiko Carstens wrote: > Fine with me. So we can eiter take Mikulas' patch which is ready or your > approach. > However it would be good to have this fixed within 3.10 since there is > a working fix aroung. Hm? Heiko, Mikulas, this is the code that I would like to go with. I am still running some I/O stress test with frequent queue stall error injection to ensure code coverage for time-out conditions. Looks good so far. Steffen's review is pending. I would appreciate your review and Ack's, too. Thanks a lot, Mikulas for your analysis. We dropped the ball on this one :-( Thanks, Martin [PATCH 2/2] zfcp: use wait_event_interruptible_lock_irq_timeout() The zfcp driver used to call wait_event_interruptible_timeout() in combination with some intricate and error-prone locking. Using wait_event_interruptible_lock_irq_timeout() as a replacement nicely cleans up that locking. This cleanup removes a situation that resulted in a locking imbalance in zfcp_qdio_sbal_get(). And we get rid of that crappy lock-unlock-lock sequence at the beginning of the critical section. Reported-by: Mikulas Patocka <mpatocka@xxxxxxxxxx> Reported-by: Heiko Carstens <heiko.carstens@xxxxxxxxxx> Signed-off-by: Martin Peschke <mpeschke@xxxxxxxxxxxxxxxxxx> --- drivers/s390/scsi/zfcp_qdio.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) --- a/drivers/s390/scsi/zfcp_qdio.c +++ b/drivers/s390/scsi/zfcp_qdio.c @@ -223,11 +223,9 @@ int zfcp_qdio_sbals_from_sg(struct zfcp_ static int zfcp_qdio_sbal_check(struct zfcp_qdio *qdio) { - spin_lock_irq(&qdio->req_q_lock); if (atomic_read(&qdio->req_q_free) || !(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP)) return 1; - spin_unlock_irq(&qdio->req_q_lock); return 0; } @@ -245,9 +243,8 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio { long ret; - spin_unlock_irq(&qdio->req_q_lock); - ret = wait_event_interruptible_timeout(qdio->req_q_wq, - zfcp_qdio_sbal_check(qdio), 5 * HZ); + ret = wait_event_interruptible_lock_irq_timeout(qdio->req_q_wq, + zfcp_qdio_sbal_check(qdio), qdio->req_q_lock, 5 * HZ); if (!(atomic_read(&qdio->adapter->status) & ZFCP_STATUS_ADAPTER_QDIOUP)) return -EIO; @@ -261,7 +258,6 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio zfcp_erp_adapter_reopen(qdio->adapter, 0, "qdsbg_1"); } - spin_lock_irq(&qdio->req_q_lock); return -EIO; } [PATCH 1/2] Add wait_event_interruptible_lock_irq_timeout() Provide another wait_event() function. It is a straight-forward descendant of wait_event_interruptible_timeout() and wait_event_interruptible_lock_irq(). There is a use case for this function in the zfcp device driver. Signed-off-by: Martin Peschke <mpeschke@xxxxxxxxxxxxxxxxxx> --- include/linux/wait.h | 57 +++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) --- a/include/linux/wait.h +++ b/include/linux/wait.h @@ -713,6 +713,63 @@ do { \ __ret; \ }) +#define __wait_event_interruptible_lock_irq_timeout(wq, condition, \ + lock, ret) \ +do { \ + DEFINE_WAIT(__wait); \ + \ + for (;;) { \ + prepare_to_wait(&wq, &__wait, TASK_INTERRUPTIBLE); \ + if (condition) \ + break; \ + if (signal_pending(current)) { \ + ret = -ERESTARTSYS; \ + break; \ + } \ + spin_unlock_irq(&lock); \ + ret = schedule_timeout(ret); \ + spin_lock_irq(&lock); \ + if (!ret) \ + break; \ + } \ + finish_wait(&wq, &__wait); \ +} while (0) + +/** + * wait_event_interruptible_lock_irq_timeout - sleep until a condition gets true or a timeout elapses. + * The condition is checked under the lock. This is expected + * to be called with the lock taken. + * @wq: the waitqueue to wait on + * @condition: a C expression for the event to wait for + * @lock: a locked spinlock_t, which will be released before schedule() + * and reacquired afterwards. + * @timeout: timeout, in jiffies + * + * The process is put to sleep (TASK_INTERRUPTIBLE) until the + * @condition evaluates to true or signal is received. The @condition is + * checked each time the waitqueue @wq is woken up. + * + * wake_up() has to be called after changing any variable that could + * change the result of the wait condition. + * + * This is supposed to be called while holding the lock. The lock is + * dropped before going to sleep and is reacquired afterwards. + * + * The function returns 0 if the @timeout elapsed, -ERESTARTSYS if it + * was interrupted by a signal, and the remaining jiffies otherwise + * if the condition evaluated to true before the timeout elapsed. + */ +#define wait_event_interruptible_lock_irq_timeout(wq, condition, lock, \ + timeout) \ +({ \ + int __ret = timeout; \ + \ + if (!(condition)) \ + __wait_event_interruptible_lock_irq_timeout( \ + wq, condition, lock, __ret); \ + __ret; \ +}) + /* * These are the old interfaces to sleep waiting for an event. -- To unsubscribe from this list: send the line "unsubscribe linux-s390" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html