Hi It looks ok. There is a difference - before this patch, zfcp_erp_adapter_reopen was called without req_q_lock. With this patch, it is called with the lock held. Can it cause any problems? (deadlock, sleep with spinlock or lock inversion?) I didn't find a case where it could, but I am not familiar with all the code in this driver. Mikulas On Wed, 12 Jun 2013, Martin Peschke wrote: > 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