SMP Lock struct file, filedesc and the global file list.

Seigo Tanimura (tanimura) posted the initial delta.

I've polished it quite a bit reducing the need for locking and
adapting it for KSE.

Locks:

1 mutex in each filedesc
   protects all the fields.
   protects "struct file" initialization, while a struct file
     is being changed from &badfileops -> &pipeops or something
     the filedesc should be locked.

1 mutex in each struct file
   protects the refcount fields.
   doesn't protect anything else.
   the flags used for garbage collection have been moved to
     f_gcflag which was the FILLER short, this doesn't need
     locking because the garbage collection is a single threaded
     container.
  could likely be made to use a pool mutex.

1 sx lock for the global filelist.

struct file *	fhold(struct file *fp);
        /* increments reference count on a file */

struct file *	fhold_locked(struct file *fp);
        /* like fhold but expects file to locked */

struct file *	ffind_hold(struct thread *, int fd);
        /* finds the struct file in thread, adds one reference and
                returns it unlocked */

struct file *	ffind_lock(struct thread *, int fd);
        /* ffind_hold, but returns file locked */

I still have to smp-safe the fget cruft, I'll get to that asap.

7 files changed, 52 insertions, 14 deletions

diff --git a/sys/fs/fdescfs/fdesc_vfsops.c b/sys/fs/fdescfs/fdesc_vfsops.c
index b9b3416..541358f 100644
--- a/sys/fs/fdescfs/fdesc_vfsops.c
+++ b/sys/fs/fdescfs/fdesc_vfsops.c
@@ -175,6 +175,7 @@ fdesc_statfs(mp, sbp, td)
 	 */
 	lim = td->td_proc->p_rlimit[RLIMIT_NOFILE].rlim_cur;
 	fdp = td->td_proc->p_fd;
+	FILEDESC_LOCK(fdp);
 	last = min(fdp->fd_nfiles, lim);
 	freefd = 0;
 	for (i = fdp->fd_freefile; i < last; i++)
@@ -187,6 +188,7 @@ fdesc_statfs(mp, sbp, td)
 	 */
 	if (fdp->fd_nfiles < lim)
 		freefd += (lim - fdp->fd_nfiles);
+	FILEDESC_UNLOCK(fdp);
 
 	sbp->f_flags = 0;
 	sbp->f_bsize = DEV_BSIZE;
diff --git a/sys/fs/fdescfs/fdesc_vnops.c b/sys/fs/fdescfs/fdesc_vnops.c
index 7cd2fee..eb6f0db 100644
--- a/sys/fs/fdescfs/fdesc_vnops.c
+++ b/sys/fs/fdescfs/fdesc_vnops.c
@@ -174,8 +174,8 @@ fdesc_lookup(ap)
 	struct componentname *cnp = ap->a_cnp;
 	char *pname = cnp->cn_nameptr;
 	struct thread *td = cnp->cn_thread;
+	struct file *fp;
 	int nlen = cnp->cn_namelen;
-	int nfiles = td->td_proc->p_fd->fd_nfiles;
 	u_int fd;
 	int error;
 	struct vnode *fvp;
@@ -212,12 +212,14 @@ fdesc_lookup(ap)
 		fd = 10 * fd + *pname++ - '0';
 	}
 
-	if (fd >= nfiles || td->td_proc->p_fd->fd_ofiles[fd] == NULL) {
+	fp = ffind_hold(td, fd);
+	if (fp == NULL) {
 		error = EBADF;
 		goto bad;
 	}
 
 	error = fdesc_allocvp(Fdesc, FD_DESC+fd, dvp->v_mount, &fvp, td);
+	fdrop(fp, td);
 	if (error)
 		goto bad;
 	VTOFDESC(fvp)->fd_fd = fd;
@@ -268,7 +270,6 @@ fdesc_getattr(ap)
 {
 	struct vnode *vp = ap->a_vp;
 	struct vattr *vap = ap->a_vap;
-	struct filedesc *fdp = ap->a_td->td_proc->p_fd;
 	struct file *fp;
 	struct stat stb;
 	u_int fd;
@@ -299,11 +300,13 @@ fdesc_getattr(ap)
 	case Fdesc:
 		fd = VTOFDESC(vp)->fd_fd;
 
-		if (fd >= fdp->fd_nfiles || (fp = fdp->fd_ofiles[fd]) == NULL)
+		fp = ffind_hold(ap->a_td, fd);
+		if (fp == NULL)
 			return (EBADF);
 
 		bzero(&stb, sizeof(stb));
 		error = fo_stat(fp, &stb, ap->a_td);
+		fdrop(fp, ap->a_td);
 		if (error == 0) {
 			VATTR_NULL(vap);
 			vap->va_type = IFTOVT(stb.st_mode);
@@ -396,10 +399,13 @@ fdesc_setattr(ap)
 		return (error);
 	}
 	vp = (struct vnode *)fp->f_data;
-	if ((error = vn_start_write(vp, &mp, V_WAIT | PCATCH)) != 0)
+	if ((error = vn_start_write(vp, &mp, V_WAIT | PCATCH)) != 0) {
+		fdrop(fp, ap->a_td);
 		return (error);
+	}
 	error = VOP_SETATTR(vp, ap->a_vap, ap->a_cred, ap->a_td);
 	vn_finished_write(mp);
+	fdrop(fp, ap->a_td);
 	return (error);
 }
 
@@ -442,6 +448,7 @@ fdesc_readdir(ap)
 
 	fcnt = i - 2;		/* The first two nodes are `.' and `..' */
 
+	FILEDESC_LOCK(fdp);
 	while (i < fdp->fd_nfiles + 2 && uio->uio_resid >= UIO_MX) {
 		switch (i) {
 		case 0:	/* `.' */
@@ -456,8 +463,10 @@ fdesc_readdir(ap)
 			dp->d_type = DT_DIR;
 			break;
 		default:
-			if (fdp->fd_ofiles[fcnt] == NULL)
+			if (fdp->fd_ofiles[fcnt] == NULL) {
+				FILEDESC_UNLOCK(fdp);
 				goto done;
+			}
 
 			bzero((caddr_t) dp, UIO_MX);
 			dp->d_namlen = sprintf(dp->d_name, "%d", fcnt);
@@ -469,12 +478,15 @@ fdesc_readdir(ap)
 		/*
 		 * And ship to userland
 		 */
+		FILEDESC_UNLOCK(fdp);
 		error = uiomove((caddr_t) dp, UIO_MX, uio);
 		if (error)
-			break;
+			goto done;
+		FILEDESC_LOCK(fdp);
 		i++;
 		fcnt++;
 	}
+	FILEDESC_UNLOCK(fdp);
 
 done:
 	uio->uio_offset = i * UIO_MX;
diff --git a/sys/fs/fifofs/fifo_vnops.c b/sys/fs/fifofs/fifo_vnops.c
index 461f7c4..3597cbb 100644
--- a/sys/fs/fifofs/fifo_vnops.c
+++ b/sys/fs/fifofs/fifo_vnops.c
@@ -344,23 +344,29 @@ fifo_ioctl(ap)
 	} */ *ap;
 {
 	struct file filetmp;
-	int error;
+	int error = 0;
 
 	if (ap->a_command == FIONBIO)
 		return (0);
+	mtx_init(&filetmp.f_mtx, "struct file", MTX_DEF);
+	filetmp.f_count = 1;
 	if (ap->a_fflag & FREAD) {
+		/* filetmp is local, hence not need be locked. */
 		filetmp.f_data = (caddr_t)ap->a_vp->v_fifoinfo->fi_readsock;
 		error = soo_ioctl(&filetmp, ap->a_command, ap->a_data, ap->a_td);
 		if (error)
-			return (error);
+			goto err;
 	}
 	if (ap->a_fflag & FWRITE) {
+		/* filetmp is local, hence not need be locked. */
 		filetmp.f_data = (caddr_t)ap->a_vp->v_fifoinfo->fi_writesock;
 		error = soo_ioctl(&filetmp, ap->a_command, ap->a_data, ap->a_td);
 		if (error)
-			return (error);
+			goto err;
 	}
-	return (0);
+err:
+	mtx_destroy(&filetmp.f_mtx);
+	return (error);
 }
 
 /* ARGSUSED */
@@ -459,6 +465,8 @@ fifo_poll(ap)
 	struct file filetmp;
 	int revents = 0;
 
+	mtx_init(&filetmp.f_mtx, "struct file", MTX_DEF);
+	filetmp.f_count = 1;
 	if (ap->a_events & (POLLIN | POLLPRI | POLLRDNORM | POLLRDBAND)) {
 		filetmp.f_data = (caddr_t)ap->a_vp->v_fifoinfo->fi_readsock;
 		if (filetmp.f_data)
@@ -471,6 +479,7 @@ fifo_poll(ap)
 			revents |= soo_poll(&filetmp, ap->a_events, ap->a_cred,
 			    ap->a_td);
 	}
+	mtx_destroy(&filetmp.f_mtx);
 	return (revents);
 }
 
diff --git a/sys/fs/portalfs/portal_vfsops.c b/sys/fs/portalfs/portal_vfsops.c
index 4e73131..bc71fa5 100644
--- a/sys/fs/portalfs/portal_vfsops.c
+++ b/sys/fs/portalfs/portal_vfsops.c
@@ -132,7 +132,8 @@ portal_mount(mp, path, data, ndp, td)
 	VTOPORTAL(rvp)->pt_size = 0;
 	VTOPORTAL(rvp)->pt_fileid = PORTAL_ROOTFILEID;
 	fmp->pm_root = rvp;
-	fmp->pm_server = fp; fp->f_count++;
+	fhold(fp);
+	fmp->pm_server = fp;
 
 	mp->mnt_flag |= MNT_LOCAL;
 	mp->mnt_data = (qaddr_t) fmp;
@@ -159,6 +160,7 @@ portal_unmount(mp, mntflags, td)
 	struct thread *td;
 {
 	int error, flags = 0;
+	struct socket *so;
 
 
 	if (mntflags & MNT_FORCE)
diff --git a/sys/fs/portalfs/portal_vnops.c b/sys/fs/portalfs/portal_vnops.c
index dd90516..e8feeb1 100644
--- a/sys/fs/portalfs/portal_vnops.c
+++ b/sys/fs/portalfs/portal_vnops.c
@@ -402,12 +402,18 @@ portal_open(ap)
 	 * Check that the mode the file is being opened for is a subset
 	 * of the mode of the existing descriptor.
 	 */
- 	fp = td->td_proc->p_fd->fd_ofiles[fd];
+	fp = ffind_hold(td, fd);
+	if (fp == NULL) {
+		error = EBADF;
+		goto bad;
+	}
 	if (((ap->a_mode & (FREAD|FWRITE)) | fp->f_flag) != fp->f_flag) {
+		fdrop(fp, td);
 		portal_closefd(td, fd);
 		error = EACCES;
 		goto bad;
 	}
+	fdrop(fp, td);
 
 	/*
 	 * Save the dup fd in the proc structure then return the
diff --git a/sys/fs/unionfs/union_subr.c b/sys/fs/unionfs/union_subr.c
index 8320f4a..70a9f52 100644
--- a/sys/fs/unionfs/union_subr.c
+++ b/sys/fs/unionfs/union_subr.c
@@ -1048,10 +1048,13 @@ union_vn_create(vpp, un, td)
 	struct vattr *vap = &vat;
 	int fmode = FFLAGS(O_WRONLY|O_CREAT|O_TRUNC|O_EXCL);
 	int error;
-	int cmode = UN_FILEMODE & ~td->td_proc->p_fd->fd_cmask;
+	int cmode;
 	struct componentname cn;
 
 	*vpp = NULLVP;
+	FILEDESC_LOCK(td->td_proc->p_fd);
+	cmode = UN_FILEMODE & ~td->td_proc->p_fd->fd_cmask;
+	FILEDESC_UNLOCK(td->td_proc->p_fd);
 
 	/*
 	 * Build a new componentname structure (for the same
@@ -1323,8 +1326,10 @@ union_dircheck(struct thread *td, struct vnode **vp, struct file *fp)
 				return (error);
 			}
 			VOP_UNLOCK(lvp, 0, td);
+			FILE_LOCK(fp);
 			fp->f_data = (caddr_t) lvp;
 			fp->f_offset = 0;
+			FILE_UNLOCK(fp);
 			error = vn_close(*vp, FREAD, fp->f_cred, td);
 			if (error)
 				return (error);
diff --git a/sys/fs/unionfs/union_vfsops.c b/sys/fs/unionfs/union_vfsops.c
index 7f3d5bd..92f46d5 100644
--- a/sys/fs/unionfs/union_vfsops.c
+++ b/sys/fs/unionfs/union_vfsops.c
@@ -224,7 +224,9 @@ union_mount(mp, path, data, ndp, td)
 	}
 
 	um->um_cred = crhold(td->td_proc->p_ucred);
+	FILEDESC_LOCK(td->td_proc->p_fd);
 	um->um_cmode = UN_DIRMODE &~ td->td_proc->p_fd->fd_cmask;
+	FILEDESC_UNLOCK(td->td_proc->p_fd);
 
 	/*
 	 * Depending on what you think the MNT_LOCAL flag might mean,