SCSI: zfcp: fix lock imbalance by reworking request queue locking

commit d79ff142624e1be080ad8d09101f7004d79c36e1 upstream.

This patch adds wait_event_interruptible_lock_irq_timeout(), which is a
straight-forward descendant of wait_event_interruptible_timeout() and
wait_event_interruptible_lock_irq().

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 rework removes a situation that resulted in a locking imbalance
in zfcp_qdio_sbal_get():

BUG: workqueue leaked lock or atomic: events/1/0xffffff00/10
    last function: zfcp_fc_wka_port_offline+0x0/0xa0 [zfcp]

It was introduced by commit c2af7545aaff3495d9bf9a7608c52f0af86fb194
"[SCSI] zfcp: Do not wait for SBALs on stopped queue", which had a new
code path related to ZFCP_STATUS_ADAPTER_QDIOUP that took an early exit
without a required lock being held. The problem occured when a
special, non-SCSI I/O request was being submitted in process context,
when the adapter's queues had been torn down. In this case the bug
surfaced when the Fibre Channel port connection for a well-known address
was closed during a concurrent adapter shut-down procedure, which is a
rare constellation.

This patch also fixes these warnings from the sparse tool (make C=1):

drivers/s390/scsi/zfcp_qdio.c:224:12: warning: context imbalance in
 'zfcp_qdio_sbal_check' - wrong count at exit
drivers/s390/scsi/zfcp_qdio.c:244:5: warning: context imbalance in
 'zfcp_qdio_sbal_get' - unexpected unlock

Last but not least, we get rid of that crappy lock-unlock-lock
sequence at the beginning of the critical section.

It is okay to call zfcp_erp_adapter_reopen() with req_q_lock held.

Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Reported-by: Heiko Carstens <heiko.carstens@de.ibm.com>
Signed-off-by: Martin Peschke <mpeschke@linux.vnet.ibm.com>
Signed-off-by: Steffen Maier <maier@linux.vnet.ibm.com>
Signed-off-by: James Bottomley <JBottomley@Parallels.com>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

diff --git a/drivers/s390/scsi/zfcp_qdio.c b/drivers/s390/scsi/zfcp_qdio.c
index 665e3cfaaf85d5b14859465163a3867073f2c5f5..de0598eaacd226ed2824ff94412069ed855a6420 100644 (file)
--- a/drivers/s390/scsi/zfcp_qdio.c
+++ b/drivers/s390/scsi/zfcp_qdio.c
@@ -224,11 +224,9 @@ int zfcp_qdio_sbals_from_sg(struct zfcp_qdio *qdio, struct zfcp_qdio_req *q_req,
 
 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;
 }
 
@@ -246,9 +244,8 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio *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;
@@ -262,7 +259,6 @@ int zfcp_qdio_sbal_get(struct zfcp_qdio *qdio)
                zfcp_erp_adapter_reopen(qdio->adapter, 0, "qdsbg_1");
        }
 
-       spin_lock_irq(&qdio->req_q_lock);
        return -EIO;
 }
 

diff --git a/include/linux/wait.h b/include/linux/wait.h
index 1133695eb0671d7aeb7f9140bf2b6adc3bc7483d..c8e5760222342669081979e1ed36623dedde59ff 100644 (file)
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -805,6 +805,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.