[linux-cifs-client] Re: [PATCH] cifs: fix wait_for_response to time out sleeping processes correctly

[wasrshi nimara]

Hi jeff,

WIll check this and get back you. Sorry for not responding earlier. Away on
vacation:)

I will also post my patch just in case.

warshi

On Tue, Nov 25, 2008 at 10:32 AM, Jeff Layton <jlayton at redhat.com> wrote:

> The current scheme that CIFS uses to sleep and wait for a response is
> not quite what we want. After sending a request, wait_for_response puts
> the task to sleep with wait_event(). One of the conditions for
> wait_event is a timeout (using time_after()).
>
> The problem with this is that there is no guarantee that the process
> will ever be woken back up. If the server stops sending data, then
> cifs_demultiplex_thread will leave its response queue sleeping.
>
> I think the only thing that saves us here is the fact that
> cifs_dnotify_thread periodically (every 15s) wakes up sleeping processes
> on all response_q's that have calls in flight.  This makes for
> unnecessary wakeups of some processes. It also means large variability
> in the timeouts since they're all woken up at once.
>
> Instead of this, put the tasks to sleep with wait_event_timeout. This
> makes them wake up on their own if they time out. With this change,
> cifs_dnotify_thread should no longer be needed.
>
> I've been testing this in conjunction with some other patches that I'm
> working on. It doesn't seem to affect performance at all with with heavy
> I/O. Identical iozone -ac runs complete in almost exactly the same time
> (<1% difference in times).
>
> Thanks to Wasrshi Nimara for initially pointing this out. Wasrshi, it
> would be nice to know whether this patch also helps your testcase.
>
> Signed-off-by: Jeff Layton <jlayton at redhat.com>
> Cc: Wasrshi Nimara <warshinimara at gmail.com>
> ---
>  fs/cifs/cifsfs.c    |   37 -------------------------------------
>  fs/cifs/transport.c |    7 ++-----
>  2 files changed, 2 insertions(+), 42 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 7f87066..1bd03af 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -66,7 +66,6 @@ unsigned int sign_CIFS_PDUs = 1;
>  extern struct task_struct *oplockThread; /* remove sparse warning */
>  struct task_struct *oplockThread = NULL;
>  /* extern struct task_struct * dnotifyThread; remove sparse warning */
> -static struct task_struct *dnotifyThread = NULL;
>  static const struct super_operations cifs_super_ops;
>  unsigned int CIFSMaxBufSize = CIFS_MAX_MSGSIZE;
>  module_param(CIFSMaxBufSize, int, 0);
> @@ -1049,32 +1048,6 @@ static int cifs_oplock_thread(void *dummyarg)
>        return 0;
>  }
>
> -static int cifs_dnotify_thread(void *dummyarg)
> -{
> -       struct list_head *tmp;
> -       struct TCP_Server_Info *server;
> -
> -       do {
> -               if (try_to_freeze())
> -                       continue;
> -               set_current_state(TASK_INTERRUPTIBLE);
> -               schedule_timeout(15*HZ);
> -               /* check if any stuck requests that need
> -                  to be woken up and wakeq so the
> -                  thread can wake up and error out */
> -               read_lock(&cifs_tcp_ses_lock);
> -               list_for_each(tmp, &cifs_tcp_ses_list) {
> -                       server = list_entry(tmp, struct TCP_Server_Info,
> -                                        tcp_ses_list);
> -                       if (atomic_read(&server->inFlight))
> -                               wake_up_all(&server->response_q);
> -               }
> -               read_unlock(&cifs_tcp_ses_lock);
> -       } while (!kthread_should_stop());
> -
> -       return 0;
> -}
> -
>  static int __init
>  init_cifs(void)
>  {
> @@ -1151,17 +1124,8 @@ init_cifs(void)
>                goto out_unregister_dfs_key_type;
>        }
>
> -       dnotifyThread = kthread_run(cifs_dnotify_thread, NULL,
> "cifsdnotifyd");
> -       if (IS_ERR(dnotifyThread)) {
> -               rc = PTR_ERR(dnotifyThread);
> -               cERROR(1, ("error %d create dnotify thread", rc));
> -               goto out_stop_oplock_thread;
> -       }
> -
>        return 0;
>
> - out_stop_oplock_thread:
> -       kthread_stop(oplockThread);
>  out_unregister_dfs_key_type:
>  #ifdef CONFIG_CIFS_DFS_UPCALL
>        unregister_key_type(&key_type_dns_resolver);
> @@ -1200,7 +1164,6 @@ exit_cifs(void)
>        cifs_destroy_mids();
>        cifs_destroy_request_bufs();
>        kthread_stop(oplockThread);
> -       kthread_stop(dnotifyThread);
>  }
>
>  MODULE_AUTHOR("Steve French <sfrench at us.ibm.com>");
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 8e0d1c3..6c1e10c 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -410,11 +410,8 @@ static int wait_for_response(struct cifsSesInfo *ses,
>
>        for (;;) {
>                curr_timeout = timeout + jiffies;
> -               wait_event(ses->server->response_q,
> -                       (!(midQ->midState == MID_REQUEST_SUBMITTED)) ||
> -                       time_after(jiffies, curr_timeout) ||
> -                       ((ses->server->tcpStatus != CifsGood) &&
> -                        (ses->server->tcpStatus != CifsNew)));
> +               wait_event_timeout(ses->server->response_q,
> +                       midQ->midState != MID_REQUEST_SUBMITTED, timeout);
>
>                if (time_after(jiffies, curr_timeout) &&
>                        (midQ->midState == MID_REQUEST_SUBMITTED) &&
> --
> 1.5.5.1
>
>
-------------- next part --------------
HTML attachment scrubbed and removed