[linux-cifs-client] Cannot allocate memory

Commit Message

Pavel Shilovsky March 11, 2009, 2:05 p.m. UTC

Jeff Layton wrote:
> You have no guarantee that "server" is still a valid pointer when you
> go to dereference it. In most cases it probably will be, but hard to
> hit races are *worse* since they're harder to track down.
>
> We have to *know* that this pointer will be valid here before you do
> something like this. Rather than adding arbitrary delays like this, a
> better scheme would use a completion variable to indicate when the
> thread is down.
>   
Yes, you are right. I fixed it.
> Finally, I'm not convinced of the need for this at all. Why should we
> guarantee that the module is able to be unplugged the instant that the
> last mount vanishes? I'd much rather my umounts return quickly rather
> than have them wait for the thread to come down.

I want to have cifs module not in use when I unmounted all share bases. 
I need it, for example, when I right any script that unmouts all shares 
and then deletes cifs module. In your solution, I have to wait some 
unknown time before deleting module. This is uncomfortably, I think. 
It's much more logical to think that all stuff connected with share 
disappears as soon as unmount finishes it's work.

Here is my new patch:





--
Best regards,
Pavel Shilovsky.

Comments

Jeff Layton March 11, 2009, 2:19 p.m. UTC | #1

On Wed, 11 Mar 2009 17:05:00 +0300
Pavel Shilovsky <[e-mail]> wrote:

> Jeff Layton wrote:
> > You have no guarantee that "server" is still a valid pointer when you
> > go to dereference it. In most cases it probably will be, but hard to
> > hit races are *worse* since they're harder to track down.
> >
> > We have to *know* that this pointer will be valid here before you do
> > something like this. Rather than adding arbitrary delays like this, a
> > better scheme would use a completion variable to indicate when the
> > thread is down.
> >   
> Yes, you are right. I fixed it.
> > Finally, I'm not convinced of the need for this at all. Why should we
> > guarantee that the module is able to be unplugged the instant that the
> > last mount vanishes? I'd much rather my umounts return quickly rather
> > than have them wait for the thread to come down.
> 
> I want to have cifs module not in use when I unmounted all share bases. 
> I need it, for example, when I right any script that unmouts all shares 
> and then deletes cifs module. In your solution, I have to wait some 
> unknown time before deleting module. This is uncomfortably, I think. 
> It's much more logical to think that all stuff connected with share 
> disappears as soon as unmount finishes it's work.
> 

Ok, fair enough. I'm not a fan of unplugging modules like this (except when testing), but whatever makes you happy...

> Here is my new patch:
> 
> diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
> index 9fbf4df..0ec5a2f 100644
> --- a/fs/cifs/cifsglob.h
> +++ b/fs/cifs/cifsglob.h
> @@ -182,6 +182,7 @@ struct TCP_Server_Info {
>      struct mac_key mac_signing_key;
>      char ntlmv2_hash[16];
>      unsigned long lstrp; /* when we got last response from this server */
> +    int running;
>  };
>  
>  /*
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index cd4ccc8..bc5841d 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -337,6 +337,7 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server)
>      bool isMultiRsp;
>      int reconnect;
>  
> +    server->running = 1;
>      current->flags |= PF_MEMALLOC;
>      cFYI(1, ("Demultiplex PID: %d", task_pid_nr(current)));
>  
> @@ -747,7 +748,6 @@ multi_t2_fnd:
>  
>      kfree(server->hostname);
>      task_to_wake = xchg(&server->tsk, NULL);
> -    kfree(server);
>  
>      length = atomic_dec_return(&tcpSesAllocCount);
>      if (length  > 0)
> @@ -764,6 +764,8 @@ multi_t2_fnd:
>          set_current_state(TASK_RUNNING);
>      }
>  
> +    server->running = 0;
> +
>      module_put_and_exit(0);
>  }
>  
> @@ -1418,6 +1420,11 @@ cifs_put_tcp_session(struct TCP_Server_Info *server)
>      task = xchg(&server->tsk, NULL);
>      if (task)
>          force_sig(SIGKILL, task);
> +
> +    while(server->running == 1)
> +        msleep(10);

^^^ this is pretty yucky though. Polling and sleeping isn't the way to do this. A completion variable + wait_for_completion() might be a better fit.

> +
> +    kfree(server);
>  }
>  
>  static struct TCP_Server_Info *
> 
> 
> 
> 
> --
> Best regards,
> Pavel Shilovsky.

Patch

diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index 9fbf4df..0ec5a2f 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -182,6 +182,7 @@  struct TCP_Server_Info {
     struct mac_key mac_signing_key;
     char ntlmv2_hash[16];
     unsigned long lstrp; /* when we got last response from this server */
+    int running;
 };
 
 /*
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index cd4ccc8..bc5841d 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -337,6 +337,7 @@  cifs_demultiplex_thread(struct TCP_Server_Info *server)
     bool isMultiRsp;
     int reconnect;
 
+    server->running = 1;
     current->flags |= PF_MEMALLOC;
     cFYI(1, ("Demultiplex PID: %d", task_pid_nr(current)));
 
@@ -747,7 +748,6 @@  multi_t2_fnd:
 
     kfree(server->hostname);
     task_to_wake = xchg(&server->tsk, NULL);
-    kfree(server);
 
     length = atomic_dec_return(&tcpSesAllocCount);
     if (length  > 0)
@@ -764,6 +764,8 @@  multi_t2_fnd:
         set_current_state(TASK_RUNNING);
     }
 
+    server->running = 0;
+
     module_put_and_exit(0);
 }
 
@@ -1418,6 +1420,11 @@  cifs_put_tcp_session(struct TCP_Server_Info *server)
     task = xchg(&server->tsk, NULL);
     if (task)
         force_sig(SIGKILL, task);
+
+    while(server->running == 1)
+        msleep(10);
+
+    kfree(server);
 }
 
 static struct TCP_Server_Info *