Re: [PATCH] Re: Locking in src/sys/dev/fss.c

Emmanuel Dreyfus <[email protected]>
Newsgroups gmane.os.netbsd.devel.kernel
Message-ID <Zra4BSMYc/[email protected]> 
On Fri, Aug 09, 2024 at 07:29:52PM +0000, Taylor R Campbell wrote:
> I don't know what the locking protocol is, but any changes to it
> absolutely must be accompanied by comments identifying the lock order
> and, if applicable, any rules like `forbidden in softint'.

Right, I go for not changing it. 

> I don't understand, how did you come to this conclusion?

Now I retry to reproduce the problem and I cannot get it
to show up. But I was hit by it a long time ago, I should
have check what I recalled before starting this async
sapshot creation thread thing.

Let us forget about the snapshot creatin thread, it seems there
are many locks missing. Attached patch tries to address that.

I wonder if I should not add a vfs_busy/vfs_unbusy in such situation:
        mount = vp->v_mount;
(...)
        vrele(vp);
(...)
        check for another vnode's ->v_mount == mount

-- 
Emmanuel Dreyfus
[email protected]
fss2.patch (text/x-diff, 8.2 KB) 
Index: fss.c
===================================================================
RCS file: /cvsroot/src/sys/dev/fss.c,v
retrieving revision 1.114
diff -U4 -r1.114 fss.c
--- fss.c	22 Mar 2023 21:14:46 -0000	1.114
+++ fss.c	10 Aug 2024 00:28:37 -0000
@@ -219,9 +219,12 @@
 		if (sc == NULL) {
 			mutex_exit(&fss_device_lock);
 			return ENOMEM;
 		}
+
+		mutex_enter(&sc->sc_slock);
 		sc->sc_state = FSS_IDLE;
+		mutex_exit(&sc->sc_slock);
 	}
 
 	mutex_enter(&sc->sc_slock);
 
@@ -359,11 +362,19 @@
 			error = EPERM;
 		if (error == 0 && sc->sc_state != FSS_IDLE) {
 			error = EBUSY;
 		} else {
+			char mntname[MNAMELEN];
+
 			sc->sc_state = FSS_CREATING;
-			copyinstr(fss->fss_mount, sc->sc_mntname,
-			    sizeof(sc->sc_mntname), NULL);
+			mutex_exit(&sc->sc_slock);
+
+			copyinstr(fss->fss_mount, mntname,
+			    sizeof(mntname), NULL);
+
+			mutex_enter(&sc->sc_slock);
+			memcpy(sc->sc_mntname, mntname,
+			    sizeof(sc->sc_mntname));
 			memset(&sc->sc_time, 0, sizeof(sc->sc_time));
 			sc->sc_clshift = 0;
 		}
 		mutex_exit(&sc->sc_slock);
@@ -701,13 +712,13 @@
 	struct timespec ts;
 	/* distinguish lookup 1 from lookup 2 to reduce mistakes */
 	struct pathbuf *pb2;
 	struct vnode *vp, *vp2;
+	struct mount *mount;
 
 	/*
 	 * Get the mounted file system.
 	 */
-
 	error = namei_simple_user(fss->fss_mount,
 				NSM_FOLLOW_NOEMULROOT, &vp);
 	if (error != 0)
 		return error;
@@ -716,10 +727,13 @@
 		vrele(vp);
 		return EINVAL;
 	}
 
-	sc->sc_mount = vp->v_mount;
-	memcpy(sc->sc_mntname, sc->sc_mount->mnt_stat.f_mntonname, MNAMELEN);
+	mount = vp->v_mount;
+	mutex_enter(&sc->sc_slock);
+	sc->sc_mount = mount;
+	memcpy(sc->sc_mntname, mount->mnt_stat.f_mntonname, MNAMELEN);
+	mutex_exit(&sc->sc_slock);
 
 	vrele(vp);
 
 	/*
@@ -730,23 +744,29 @@
 				NSM_FOLLOW_NOEMULROOT, &vp);
 	if (error != 0)
 		return error;
 
-	if (vp->v_type == VREG && vp->v_mount == sc->sc_mount) {
+	mutex_enter(&sc->sc_slock);
+	if (vp->v_type == VREG && vp->v_mount == mount) {
 		sc->sc_flags |= FSS_PERSISTENT;
 		sc->sc_bs_vp = vp;
 
-		fsbsize = sc->sc_bs_vp->v_mount->mnt_stat.f_iosize;
+		fsbsize = mount->mnt_stat.f_iosize;
 		bits = sizeof(sc->sc_bs_bshift)*NBBY;
+
 		for (sc->sc_bs_bshift = 1; sc->sc_bs_bshift < bits;
 		    sc->sc_bs_bshift++)
 			if (FSS_FSBSIZE(sc) == fsbsize)
 				break;
-		if (sc->sc_bs_bshift >= bits)
+
+		if (sc->sc_bs_bshift >= bits) {
+			mutex_exit(&sc->sc_slock);
 			return EINVAL;
+		}
 
 		sc->sc_bs_bmask = FSS_FSBSIZE(sc)-1;
 		sc->sc_clshift = 0;
+		mutex_exit(&sc->sc_slock);
 
 		if ((fss->fss_flags & FSS_UNLINK_ON_CREATE) != 0) {
 			error = do_sys_unlink(fss->fss_bstore, UIO_USERSPACE);
 			if (error)
@@ -754,12 +774,14 @@
 		}
 		error = vn_lock(vp, LK_EXCLUSIVE);
 		if (error != 0)
 			return error;
-		error = VFS_SNAPSHOT(sc->sc_mount, sc->sc_bs_vp, &ts);
+		error = VFS_SNAPSHOT(mount, vp, &ts);
+		mutex_enter(&sc->sc_slock);
 		TIMESPEC_TO_TIMEVAL(&sc->sc_time, &ts);
+		mutex_exit(&sc->sc_slock);
 
-		VOP_UNLOCK(sc->sc_bs_vp);
+		VOP_UNLOCK(vp);
 
 		return error;
 	}
 	vrele(vp);
@@ -767,12 +789,15 @@
 	/*
 	 * Get the block device it is mounted on and its size.
 	 */
 
-	error = spec_node_lookup_by_mount(sc->sc_mount, &vp);
+	error = spec_node_lookup_by_mount(mount, &vp);
 	if (error)
 		return error;
+
+	mutex_enter(&sc->sc_slock);
 	sc->sc_bdev = vp->v_rdev;
+	mutex_exit(&sc->sc_slock);
 
 	error = getdisksize(vp, &numsec, &secsize);
 	vrele(vp);
 	if (error)
@@ -784,19 +809,21 @@
 	 * Get the backing store
 	 */
 
 	error = pathbuf_copyin(fss->fss_bstore, &pb2);
-	if (error) {
+	if (error)
  		return error;
-	}
+
 	error = vn_open(NULL, pb2, 0, FREAD|FWRITE, 0, &vp2, NULL, NULL);
 	if (error != 0) {
 		pathbuf_destroy(pb2);
 		return error;
 	}
 	VOP_UNLOCK(vp2);
 
+	mutex_enter(&sc->sc_slock);
 	sc->sc_bs_vp = vp2;
+	mutex_exit(&sc->sc_slock);
 
 	if (vp2->v_type != VREG && vp2->v_type != VCHR) {
 		vrele(vp2);
 		pathbuf_destroy(pb2);
@@ -808,23 +835,30 @@
 		error = do_sys_unlink(fss->fss_bstore, UIO_USERSPACE);
 		if (error)
 			return error;
 	}
+
+	mutex_enter(&sc->sc_slock);
 	if (sc->sc_bs_vp->v_type == VREG) {
 		fsbsize = sc->sc_bs_vp->v_mount->mnt_stat.f_iosize;
-		if (fsbsize & (fsbsize-1))	/* No power of two */
+		if (fsbsize & (fsbsize-1)) {	/* No power of two */
+			mutex_exit(&sc->sc_slock);
 			return EINVAL;
+		}
 		for (sc->sc_bs_bshift = 1; sc->sc_bs_bshift < 32;
 		    sc->sc_bs_bshift++)
 			if (FSS_FSBSIZE(sc) == fsbsize)
 				break;
-		if (sc->sc_bs_bshift >= 32)
+		if (sc->sc_bs_bshift >= 32) {
+			mutex_exit(&sc->sc_slock);
 			return EINVAL;
+		}
 		sc->sc_bs_bmask = FSS_FSBSIZE(sc)-1;
 	} else {
 		sc->sc_bs_bshift = DEV_BSHIFT;
 		sc->sc_bs_bmask = FSS_FSBSIZE(sc)-1;
 	}
+	mutex_exit(&sc->sc_slock);
 
 	return 0;
 }
 
@@ -833,8 +867,10 @@
  */
 static int
 fss_create_snapshot(struct fss_softc *sc, struct fss_set *fss, struct lwp *l)
 {
+	struct mount *mount = sc->sc_mount;
+	struct vnode *vp;
 	int len, error;
 	u_int32_t csize;
 	off_t bsize;
 
@@ -845,15 +881,16 @@
 	 */
 	if ((error = fss_create_files(sc, fss, &bsize, l)) != 0)
 		goto bad;
 
+	mutex_enter(&sc->sc_slock);
 	if (sc->sc_flags & FSS_PERSISTENT) {
-		fss_softc_alloc(sc);
-		mutex_enter(&sc->sc_slock);
 		sc->sc_state = FSS_ACTIVE;
 		mutex_exit(&sc->sc_slock);
+		fss_softc_alloc(sc);
 		return 0;
 	}
+	mutex_exit(&sc->sc_slock);
 
 	/*
 	 * Set cluster size. Must be a power of two and
 	 * a multiple of backing store block size.
@@ -864,13 +901,15 @@
 		csize = fss->fss_csize;
 	if (bsize/csize > FSS_CLUSTER_MAX)
 		csize = bsize/FSS_CLUSTER_MAX+1;
 
+	mutex_enter(&sc->sc_slock);
 	for (sc->sc_clshift = sc->sc_bs_bshift; sc->sc_clshift < 32;
 	    sc->sc_clshift++)
 		if (FSS_CLSIZE(sc) >= csize)
 			break;
 	if (sc->sc_clshift >= 32) {
+		mutex_exit(&sc->sc_slock);
 		error = EINVAL;
 		goto bad;
 	}
 	sc->sc_clmask = FSS_CLSIZE(sc)-1;
@@ -898,52 +937,67 @@
 	sc->sc_indir_size = FSS_BTOCL(sc, len)+1;
 	sc->sc_clnext = sc->sc_indir_size;
 	sc->sc_indir_cur = 0;
 
-	if ((error = fss_softc_alloc(sc)) != 0)
+	mutex_exit(&sc->sc_slock);
+
+	if ((error = fss_softc_alloc(sc)) != 0) {
 		goto bad;
+	}
 
 	/*
 	 * Activate the snapshot.
 	 */
 
-	if ((error = vfs_suspend(sc->sc_mount, 0)) != 0)
+	if ((error = vfs_suspend(mount, 0)) != 0)
 		goto bad;
 
+	mutex_enter(&sc->sc_slock);
 	microtime(&sc->sc_time);
+	mutex_exit(&sc->sc_slock);
 
-	vrele_flush(sc->sc_mount);
-	error = VFS_SYNC(sc->sc_mount, MNT_WAIT, curlwp->l_cred);
-	if (error == 0)
-		error = fscow_establish(sc->sc_mount, fss_copy_on_write, sc);
+	vrele_flush(mount);
+	error = VFS_SYNC(mount, MNT_WAIT, curlwp->l_cred);
+	if (error == 0) 
+		error = fscow_establish(mount, fss_copy_on_write, sc);
 	if (error == 0) {
 		mutex_enter(&sc->sc_slock);
 		sc->sc_state = FSS_ACTIVE;
 		mutex_exit(&sc->sc_slock);
 	}
 
-	vfs_resume(sc->sc_mount);
+	vfs_resume(mount);
 
 	if (error != 0)
 		goto bad;
 
+	mutex_enter(&sc->sc_slock);
 	aprint_debug_dev(sc->sc_dev, "%s snapshot active\n", sc->sc_mntname);
 	aprint_debug_dev(sc->sc_dev,
 	    "%u clusters of %u, %u cache slots, %u indir clusters\n",
 	    sc->sc_clcount, FSS_CLSIZE(sc),
 	    sc->sc_cache_size, sc->sc_indir_size);
+	mutex_exit(&sc->sc_slock);
 
 	return 0;
 
 bad:
 	fss_softc_free(sc);
-	if (sc->sc_bs_vp != NULL) {
-		if (sc->sc_flags & FSS_PERSISTENT)
-			vrele(sc->sc_bs_vp);
-		else
-			vn_close(sc->sc_bs_vp, FREAD|FWRITE, l->l_cred);
+
+	mutex_enter(&sc->sc_slock);
+	if ((vp = sc->sc_bs_vp) != NULL) {
+		if (sc->sc_flags & FSS_PERSISTENT) {
+			mutex_exit(&sc->sc_slock);
+			vrele(vp);
+		} else {
+			mutex_exit(&sc->sc_slock);
+			vn_close(vp, FREAD|FWRITE, l->l_cred);
+		}
 	}
+
+	mutex_enter(&sc->sc_slock);
 	sc->sc_bs_vp = NULL;
+	mutex_exit(&sc->sc_slock);
 
 	return error;
 }
 
@@ -1390,23 +1444,26 @@
 
 		error = devsw_attach(fss_cd.cd_name,
 		    &fss_bdevsw, &fss_bmajor, &fss_cdevsw, &fss_cmajor);
 		if (error) {
+			cv_destroy(&fss_device_cv);
 			mutex_destroy(&fss_device_lock);
 			break;
 		}
 
 		error = config_cfdriver_attach(&fss_cd);
 		if (error) {
 			devsw_detach(&fss_bdevsw, &fss_cdevsw);
+			cv_destroy(&fss_device_cv);
 			mutex_destroy(&fss_device_lock);
 			break;
 		}
 
 		error = config_cfattach_attach(fss_cd.cd_name, &fss_ca);
 		if (error) {
 			config_cfdriver_detach(&fss_cd);
 			devsw_detach(&fss_bdevsw, &fss_cdevsw);
+			cv_destroy(&fss_device_cv);
 			mutex_destroy(&fss_device_lock);
 			break;
 		}
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.