Return-Path: linux-nfs-owner@vger.kernel.org Received: from mx1.redhat.com ([209.132.183.28]:26593 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752167Ab3HVNw2 (ORCPT ); Thu, 22 Aug 2013 09:52:28 -0400 Date: Thu, 22 Aug 2013 09:52:24 -0400 From: Jeff Layton To: Trond Myklebust Cc: linux-nfs@vger.kernel.org, Andy Adamson , steved@redhat.com Subject: Re: [PATCH] SUNRPC: We must not use list_for_each_entry_safe() in rpc_wake_up() Message-ID: <20130822095224.13eec221@tlielax.poochiereds.net> In-Reply-To: <1332181002-10302-1-git-send-email-Trond.Myklebust@netapp.com> References: <1332181002-10302-1-git-send-email-Trond.Myklebust@netapp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-nfs-owner@vger.kernel.org List-ID: On Mon, 19 Mar 2012 14:16:42 -0400 Trond Myklebust wrote: > The problem is that for the case of priority queues, we > have to assume that __rpc_remove_wait_queue_priority will move new > elements from the tk_wait.links lists into the queue->tasks[] list. > We therefore cannot use list_for_each_entry_safe() on queue->tasks[], > since that will skip these new tasks that __rpc_remove_wait_queue_priority > is adding. > > Without this fix, rpc_wake_up and rpc_wake_up_status will both fail > to wake up all functions on priority wait queues, which can result > in some nasty hangs. > > Reported-by: Andy Adamson > Signed-off-by: Trond Myklebust > Cc: stable@vger.kernel.org > --- > net/sunrpc/sched.c | 15 +++++++++++---- > 1 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/net/sunrpc/sched.c b/net/sunrpc/sched.c > index 1c570a8..994cfea 100644 > --- a/net/sunrpc/sched.c > +++ b/net/sunrpc/sched.c > @@ -534,14 +534,18 @@ EXPORT_SYMBOL_GPL(rpc_wake_up_next); > */ > void rpc_wake_up(struct rpc_wait_queue *queue) > { > - struct rpc_task *task, *next; > struct list_head *head; > > spin_lock_bh(&queue->lock); > head = &queue->tasks[queue->maxpriority]; > for (;;) { > - list_for_each_entry_safe(task, next, head, u.tk_wait.list) > + while (!list_empty(head)) { > + struct rpc_task *task; > + task = list_first_entry(head, > + struct rpc_task, > + u.tk_wait.list); > rpc_wake_up_task_queue_locked(queue, task); > + } > if (head == &queue->tasks[0]) > break; > head--; > @@ -559,13 +563,16 @@ EXPORT_SYMBOL_GPL(rpc_wake_up); > */ > void rpc_wake_up_status(struct rpc_wait_queue *queue, int status) > { > - struct rpc_task *task, *next; > struct list_head *head; > > spin_lock_bh(&queue->lock); > head = &queue->tasks[queue->maxpriority]; > for (;;) { > - list_for_each_entry_safe(task, next, head, u.tk_wait.list) { > + while (!list_empty(head)) { > + struct rpc_task *task; > + task = list_first_entry(head, > + struct rpc_task, > + u.tk_wait.list); > task->tk_status = status; > rpc_wake_up_task_queue_locked(queue, task); > } I have a question about this (old) patch... Steve D. backported this patch for RHEL5, and it's causing a deadlock there. This is mainly due to the fact that this code in RHEL5 is based on 2.6.18 and is quite different (it still has the RPC_TASK_WAKEUP flag, and there's an inversion problem with it in the code). I don't think it's a problem in most later kernels. What I'm not clear on is this: how can new entries end up on these lists even though we hold the queue->lock here? It looks like the queue->lock is always held when __rpc_remove_wait_queue_priority is called. Even if there is some way for that to happen, how is it safe to add entries to that list without taking the lock? That seems like it would be a great way to corrupt the list. Thanks, -- Jeff Layton