On Mon, 20 Dec 2010 22:08:38 +0530 Suresh Jayaraman <[email protected]> wrote:
> On 12/17/2010 08:38 PM, Jeff Layton wrote: > > The TCP_Server_Info is refcounted and every SMB session holds a > > reference to it. Thus, smb_ses_list is always going to be empty when > > cifsd is coming down. This is dead code. > > > > Signed-off-by: Jeff Layton <[email protected]> > > --- > > fs/cifs/connect.c | 44 +++----------------------------------------- > > 1 files changed, 3 insertions(+), 41 deletions(-) > > > > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c > > index 9fe620e2..67d3d0d 100644 > > --- a/fs/cifs/connect.c > > +++ b/fs/cifs/connect.c > > @@ -346,7 +346,6 @@ cifs_demultiplex_thread(struct TCP_Server_Info *server) > > struct kvec iov; > > struct socket *csocket = server->ssocket; > > struct list_head *tmp; > > - struct cifsSesInfo *ses; > > struct task_struct *task_to_wake = NULL; > > struct mid_q_entry *mid_entry; > > char temp; > > @@ -677,44 +676,19 @@ multi_t2_fnd: > > if (smallbuf) /* no sense logging a debug message if NULL */ > > cifs_small_buf_release(smallbuf); > > > > - /* > > - * BB: we shouldn't have to do any of this. It shouldn't be > > - * possible to exit from the thread with active SMB sessions > > - */ > > - spin_lock(&cifs_tcp_ses_lock); > > - if (list_empty(&server->pending_mid_q)) { > > - /* loop through server session structures attached to this and > > - mark them dead */ > > - list_for_each(tmp, &server->smb_ses_list) { > > - ses = list_entry(tmp, struct cifsSesInfo, > > - smb_ses_list); > > - ses->status = CifsExiting; > > - ses->server = NULL; > > - } > > - spin_unlock(&cifs_tcp_ses_lock); > > - } else { > > - /* although we can not zero the server struct pointer yet, > > - since there are active requests which may depnd on them, > > - mark the corresponding SMB sessions as exiting too */ > > - list_for_each(tmp, &server->smb_ses_list) { > > - ses = list_entry(tmp, struct cifsSesInfo, > > - smb_ses_list); > > - ses->status = CifsExiting; > > - } > > - > > + if (!list_empty(&server->pending_mid_q)) { > > spin_lock(&GlobalMid_Lock); > > list_for_each(tmp, &server->pending_mid_q) { > > - mid_entry = list_entry(tmp, struct mid_q_entry, qhead); > > + mid_entry = list_entry(tmp, struct mid_q_entry, qhead); > > if (mid_entry->midState == MID_REQUEST_SUBMITTED) { > > cFYI(1, "Clearing Mid 0x%x - waking up ", > > - mid_entry->mid); > > + mid_entry->mid); > > task_to_wake = mid_entry->tsk; > > if (task_to_wake) > > wake_up_process(task_to_wake); > > } > > } > > spin_unlock(&GlobalMid_Lock); > > - spin_unlock(&cifs_tcp_ses_lock); > > /* 1/8th of sec is more than enough time for them to exit */ > > msleep(125); > > } > > @@ -732,18 +706,6 @@ multi_t2_fnd: > > coming home not much else we can do but free the memory */ > > } > > > > - /* last chance to mark ses pointers invalid > > - if there are any pointing to this (e.g > > - if a crazy root user tried to kill cifsd > > - kernel thread explicitly this might happen) */ > > The above comment sounds little scary. Though, I don't see how this > could happen, I think it's better to test the above scenario to be sure > (I guess you would have already taken care?) > > > - /* BB: This shouldn't be necessary, see above */ > > - spin_lock(&cifs_tcp_ses_lock); > > - list_for_each(tmp, &server->smb_ses_list) { > > - ses = list_entry(tmp, struct cifsSesInfo, smb_ses_list); > > - ses->server = NULL; > > - } > > - spin_unlock(&cifs_tcp_ses_lock); > > - > > kfree(server->hostname); > > task_to_wake = xchg(&server->tsk, NULL); > > kfree(server); > > That comment is a holdover from earlier, broken code that did not do proper refcounting (see the changes in 2008 near commit e7ddee90 that fixed this). You'll note that I added this comment around the same time: - /* - * BB: we shouldn't have to do any of this. It shouldn't be - * possible to exit from the thread with active SMB sessions - */ ...but I never got around to actually ripping out that bogus code. Today, the only way to take down cifs_demultiplex_thread is for the refcount (server->srv_count) to go to zero. The model is for each cifsSesInfo struct to hold a single reference to a TCP_Server_Info. So, if there are still entries on the server->smb_ses_list while the thread is coming down, then there is something very, very wrong. -- Jeff Layton <[email protected]> -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
