From owner-cvs-all Wed Jul 17 6:48:43 2002 Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.FreeBSD.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id 53DE437B406 for ; Wed, 17 Jul 2002 06:48:07 -0700 (PDT) Received: from mail.speakeasy.net (mail14.speakeasy.net [216.254.0.214]) by mx1.FreeBSD.org (Postfix) with ESMTP id 218F343E58 for ; Wed, 17 Jul 2002 06:48:06 -0700 (PDT) (envelope-from jhb@FreeBSD.org) Received: (qmail 11773 invoked from network); 17 Jul 2002 13:48:03 -0000 Received: from unknown (HELO server.baldwin.cx) ([216.27.160.63]) (envelope-sender ) by mail14.speakeasy.net (qmail-ldap-1.03) with DES-CBC3-SHA encrypted SMTP for ; 17 Jul 2002 13:48:03 -0000 Received: from laptop.baldwin.cx (laptop.baldwin.cx [192.168.0.4]) by server.baldwin.cx (8.11.6/8.11.6) with ESMTP id g6HDlq056867; Wed, 17 Jul 2002 09:47:52 -0400 (EDT) (envelope-from jhb@FreeBSD.org) Message-ID: X-Mailer: XFMail 1.5.2 on FreeBSD X-Priority: 3 (Normal) Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8bit MIME-Version: 1.0 In-Reply-To: <010701c22d63$c5203630$ef01a8c0@davidwnt> Date: Wed, 17 Jul 2002 09:48:03 -0400 (EDT) From: John Baldwin To: David Xu Subject: Re: cvs commit: src/sys/kern kern_descrip.c Cc: cvs-all@FreeBSD.ORG, cvs-committers@FreeBSD.ORG Sender: owner-cvs-all@FreeBSD.ORG Precedence: bulk List-ID: List-Archive: (Web Archive) List-Help: (List Instructions) List-Subscribe: List-Unsubscribe: X-Loop: FreeBSD.ORG On 17-Jul-2002 David Xu wrote: > ----- Original Message ----- > From: "John Baldwin" > To: ; > Sent: Wednesday, July 17, 2002 10:48 AM > Subject: cvs commit: src/sys/kern kern_descrip.c > > >> jhb 2002/07/16 19:48:43 PDT >> >> Modified files: >> sys/kern kern_descrip.c >> Log: >> Preallocate a struct file as the first thing in falloc() before we lock >> the filelist_lock and check nfiles. This closes a race where we had to >> unlock the filedesc to re-lock the filelist_lock. >> >> Reported by: David Xu >> Reviewed by: bde (mostly) >> >> Revision Changes Path >> 1.150 +5 -16 src/sys/kern/kern_descrip.c >> > > holds filelist_lock and does not release until function returns, > this lock seems too big for me. it almostly forces the whole system > to serialize through open() syscall. I see filelist_lock only maintain > global opened file list. why should a program calling falloc be blocked > out by another program calling falloc so long time? Look at the code in question that is under the lock. It may look long but it really isn't. It's mostly just doing simple variable assignments and shuffles. The only function it calls is fdalloc(). Oh, geez. And that unlocks the filedesc lock. So every place that calls fdalloc() is actually a race. *sigh* Actually, it doesn't need to. Hmm, but that can still be potentially long. Well, we could do this: Index: kern_descrip.c =================================================================== RCS file: /home/ncvs/src/sys/kern/kern_descrip.c,v retrieving revision 1.150 diff -u -r1.150 kern_descrip.c --- kern_descrip.c 17 Jul 2002 02:48:43 -0000 1.150 +++ kern_descrip.c 17 Jul 2002 13:22:43 -0000 @@ -169,7 +169,6 @@ int i, error; FILEDESC_LOCK(fdp); -retry: if (old >= fdp->fd_nfiles || fdp->fd_ofiles[old] == NULL || new >= p->p_rlimit[RLIMIT_NOFILE].rlim_cur || @@ -187,10 +186,6 @@ FILEDESC_UNLOCK(fdp); return (error); } - /* - * fdalloc() may block, retest everything. - */ - goto retry; } error = do_dup(fdp, (int)old, (int)new, td->td_retval, td); return(error); @@ -1015,25 +1010,9 @@ nfiles = NDEXTENT; else nfiles = 2 * fdp->fd_nfiles; - FILEDESC_UNLOCK(fdp); - mtx_lock(&Giant); MALLOC(newofile, struct file **, nfiles * OFILESIZE, M_FILEDESC, M_WAITOK); - mtx_unlock(&Giant); - FILEDESC_LOCK(fdp); - /* - * deal with file-table extend race that might have occured - * when malloc was blocked. - */ - if (fdp->fd_nfiles >= nfiles) { - FILEDESC_UNLOCK(fdp); - mtx_lock(&Giant); - FREE(newofile, M_FILEDESC); - mtx_unlock(&Giant); - FILEDESC_LOCK(fdp); - continue; - } newofileflags = (char *) &newofile[nfiles]; /* * Copy the existing ofile and ofileflags arrays @@ -1053,13 +1032,8 @@ fdp->fd_ofileflags = newofileflags; fdp->fd_nfiles = nfiles; fdexpand++; - if (oldofile != NULL) { - FILEDESC_UNLOCK(fdp); - mtx_lock(&Giant); + if (oldofile != NULL) FREE(oldofile, M_FILEDESC); - mtx_unlock(&Giant); - FILEDESC_LOCK(fdp); - } } return (0); } @@ -1122,28 +1096,26 @@ * descriptor to the list of open files at that point, otherwise * put it at the front of the list of open files. */ - FILEDESC_LOCK(p->p_fd); - if ((error = fdalloc(td, 0, &i))) { - FILEDESC_UNLOCK(p->p_fd); - nfiles--; - sx_xunlock(&filelist_lock); - uma_zfree(file_zone, fp); - return (error); - } fp->f_mtxp = mtx_pool_alloc(); fp->f_gcflag = 0; fp->f_count = 1; fp->f_cred = crhold(td->td_ucred); fp->f_ops = &badfileops; fp->f_seqcount = 1; + FILEDESC_LOCK(p->p_fd); if ((fq = p->p_fd->fd_ofiles[0])) { LIST_INSERT_AFTER(fq, fp, f_list); } else { LIST_INSERT_HEAD(&filehead, fp, f_list); } + sx_xunlock(&filelist_lock); + if ((error = fdalloc(td, 0, &i))) { + FILEDESC_UNLOCK(p->p_fd); + fdrop(fp, td); + return (error); + } p->p_fd->fd_ofiles[i] = fp; FILEDESC_UNLOCK(p->p_fd); - sx_xunlock(&filelist_lock); if (resultfp) *resultfp = fp; if (resultfd) This doesn't hold filelist_lock across fdalloc() and thus possibly across malloc(). It also just holds the filedesc pointer across malloc() so that fdalloc() is no longer racy (no internal races or introducing ones into calling code). -- John Baldwin <>< http://www.FreeBSD.org/~jhb/ "Power Users Use the Power to Serve!" - http://www.FreeBSD.org/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe cvs-all" in the body of the message