[linux-cifs-client] [PATCH] Cannot allocate memory

Jeff Layton jlayton at redhat.com
Wed Mar 11 14:19:47 GMT 2009


On Wed, 11 Mar 2009 17:05:00 +0300
Pavel Shilovsky <piastry at etersoft.ru> 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.


-- 
Jeff Layton <jlayton at redhat.com>


More information about the linux-cifs-client mailing list