Re: [PATCH 02/18] cifs: no need to mark smb_ses_list as cifs_demultiplex_thread is exiting

[Jeff Layton <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

On Mon, 20 Dec 2010 22:08:38 +0530
Suresh Jayaraman <sjayaraman-l3A5Bk7waGM@public.gmane.org> 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 <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> >  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 <jlayton-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>