shithub: front

Download patch

ref: 906f8f188f86e09b7f93445dd20571ad83790fed
parent: af842ca41a10f88089bd38bcd22a14b68e5aef63
author: Ori Bernstein <ori@eigenstate.org>
date: Fri May 30 09:50:53 EDT 2025 

gefs: use rwlock instead of atomics for mount list

Using an rwlock here simplifies reasoning about the code.
The individial elements of the mount list still participate
in EBR, becuase accessing them via the FID needs to be safe,
but we don't need the performance of EBR when walking the
list.


--- a/sys/src/cmd/gefs/cons.c
+++ b/sys/src/cmd/gefs/cons.c
@@ -173,7 +173,9 @@
 	for(i = 0; i < fs->nusers; i++){
 		u = &fs->users[i];
 		fprint(fd, "%d:%s:", u->id, u->name);
-		if((v = uid2user(u->lead)) == nil)
+		if(u->lead == noneid)
+			fprint(fd, ":");
+		else if((v = uid2user(u->lead)) == nil)
 			fprint(fd, "???:");
 		else
 			fprint(fd, "%s:", v->name);
--- a/sys/src/cmd/gefs/dat.h
+++ b/sys/src/cmd/gefs/dat.h
@@ -533,7 +533,7 @@
 	QLock	synclk;
 	Rendez	syncrz;
 
-	QLock	mountlk;
+	RWLock	mountlk;
 	Mount	*mounts;
 	Mount	*snapmnt;
 	Lock	connlk;
--- a/sys/src/cmd/gefs/fs.c
+++ b/sys/src/cmd/gefs/fs.c
@@ -114,8 +114,16 @@
 		nexterror();
 	}
 	tracem("packb");
-	for(mnt = agetp(&fs->mounts); mnt != nil; mnt = mnt->next)
+
+	rlock(&fs->mountlk);
+	if(waserror()){
+		runlock(&fs->mountlk);
+		nexterror();
+	}
+	for(mnt = fs->mounts; mnt != nil; mnt = mnt->next)
 		updatesnap(&mnt->root, mnt->root, mnt->name, mnt->flag);
+	runlock(&fs->mountlk);
+	poperror();
 	/*
 	 * Now that we've updated the snaps, we can sync the
 	 * dlist; the snap tree will not change from here.
@@ -204,16 +212,18 @@
 static void
 snapfs(Amsg *a, Tree **tp)
 {
-	Tree *t, *s;
+	Tree *t, *s, *r;
 	Mount *mnt;
 
+	t = nil;
+	r = nil;
+	*tp = nil;
+	rlock(&fs->mountlk);
 	if(waserror()){
-		*tp = nil;
+		runlock(&fs->mountlk);
 		nexterror();
 	}
-	t = nil;
-	*tp = nil;
-	for(mnt = agetp(&fs->mounts); mnt != nil; mnt = mnt->next){
+	for(mnt = fs->mounts; mnt != nil; mnt = mnt->next){
 		if(strcmp(a->old, mnt->name) == 0){
 			updatesnap(&mnt->root, mnt->root, mnt->name, mnt->flag);
 			t = agetp(&mnt->root);
@@ -224,6 +234,7 @@
 	if(t == nil && (t = opensnap(a->old, nil)) == nil){
 		if(a->fd != -1)
 			fprint(a->fd, "snap: open '%s': does not exist\n", a->old);
+		runlock(&fs->mountlk);
 		poperror();
 		return;
 	}
@@ -231,12 +242,13 @@
 		if(mnt != nil) {
 			if(a->fd != -1)
 				fprint(a->fd, "snap: snap is mounted: '%s'\n", a->old);
+			runlock(&fs->mountlk);
 			poperror();
 			return;
 		}
 		if(t->nlbl == 1 && t->nref <= 1 && t->succ == -1){
 			ainc(&t->memref);
-			*tp = t;
+			r = t;
 		}
 		delsnap(t, t->succ, a->old);
 	}else{
@@ -244,6 +256,7 @@
 			if(a->fd != -1)
 				fprint(a->fd, "snap: already exists '%s'\n", a->new);
 			closesnap(s);
+			runlock(&fs->mountlk);
 			poperror();
 			return;
 		}
@@ -250,7 +263,9 @@
 		tagsnap(t, a->new, a->flag);
 	}
 	closesnap(t);
+	runlock(&fs->mountlk);
 	poperror();
+	*tp = r;
 	if(a->fd != -1){
 		if(a->delete)
 			fprint(a->fd, "deleted: %s\n", a->old);
@@ -415,8 +430,16 @@
 {
 	if(!(mnt->flag & Lmut))
 		error(Erdonly);
-	if(mnt->root->nlbl != 1 || mnt->root->nref != 0)
+	if(mnt->root->nlbl != 1 || mnt->root->nref != 0){
+		rlock(&fs->mountlk);
+		if(waserror()){
+			runlock(&fs->mountlk);
+			nexterror();
+		}
 		updatesnap(&mnt->root, mnt->root, mnt->name, mnt->flag);
+		poperror();
+		runlock(&fs->mountlk);
+	}
 	btupsert(mnt->root, m, nm);
 }
 
@@ -679,22 +702,19 @@
 		return fs->snapmnt;
 	}
 
-	qlock(&fs->mountlk);
-	for(mnt = agetp(&fs->mounts); mnt != nil; mnt = mnt->next){
+	wlock(&fs->mountlk);
+	for(mnt = fs->mounts; mnt != nil; mnt = mnt->next){
 		if(strcmp(name, mnt->name) == 0){
 			ainc(&mnt->ref);
 			goto Out;
 		}
 	}
-	if((mnt = mallocz(sizeof(*mnt), 1)) == nil){
-		qunlock(&fs->mountlk);
-		error(Enomem);
-	}
 	if(waserror()){
-		qunlock(&fs->mountlk);
+		wunlock(&fs->mountlk);
 		free(mnt);
 		nexterror();
 	}
+	mnt = emalloc(sizeof(*mnt), 1);
 	mnt->ref = 1;
 	snprint(mnt->name, sizeof(mnt->name), "%s", name);
 	if((t = opensnap(name, &flg)) == nil)
@@ -703,11 +723,11 @@
 	mnt->root = t;
 	mnt->next = fs->mounts;
 	loadautos(mnt);
-	asetp(&fs->mounts, mnt);
+	fs->mounts = mnt;
 	poperror();
 
 Out:
-	qunlock(&fs->mountlk);
+	wunlock(&fs->mountlk);
 	return mnt;
 }
 
@@ -718,8 +738,8 @@
 
 	if(mnt == nil)
 		return;
+	wlock(&fs->mountlk);
 	if(adec(&mnt->ref) == 0){
-		qlock(&fs->mountlk);
 		for(p = &fs->mounts; (me = *p) != nil; p = &me->next){
 			if(me == mnt)
 				break;
@@ -727,8 +747,8 @@
 		assert(me != nil);
 		*p = me->next;
 		limbo(DFmnt, me);
-		qunlock(&fs->mountlk);
 	}
+	wunlock(&fs->mountlk);
 }
 
 static void
@@ -3032,12 +3052,14 @@
 
 		tmnow(&tm, nil);
 		now = tmnorm(&tm);
-		for(mnt = agetp(&fs->mounts); mnt != nil; mnt = mnt->next){
+		rlock(&fs->mountlk);
+		for(mnt = fs->mounts; mnt != nil; mnt = mnt->next){
 			if(!(mnt->flag & Lmut))
 				continue;
 			for(i = 0; i < nelem(mnt->cron); i++)
 				cronsync(mnt->name, &mnt->cron[i], &tm, now);
 		}
+		runlock(&fs->mountlk);
 		poperror();
 	}
 }
--- a/sys/src/cmd/gefs/snap.c
+++ b/sys/src/cmd/gefs/snap.c
@@ -310,6 +310,8 @@
  *
  * If it has one successor and no label, then
  * it will be merged with that successor.
+ *
+ * Must be called with mntlock held for r
  */
 void
 delsnap(Tree *t, vlong succ, char *name)
@@ -360,7 +362,8 @@
 	btupsert(&fs->snap, m, nm);
 	if(deltree){
 		reclaimblocks(t->gen, succ, t->pred);
-		for(mnt = agetp(&fs->mounts); mnt != nil; mnt = mnt->next){
+		assert(!canwlock(&fs->mountlk));
+		for(mnt = fs->mounts; mnt != nil; mnt = mnt->next){
 			if(mnt->root->gen == t->succ)
 				mnt->root->pred = t->pred;
 			if(mnt->root->gen == t->pred)
--- a/sys/src/cmd/gefs/user.c
+++ b/sys/src/cmd/gefs/user.c
@@ -133,6 +133,7 @@
 			goto Error;
 		}
 		snprint(u->name, sizeof(u->name), "%s", f);
+		u->lead = noneid;
 		u->memb = nil;
 		u->nmemb = 0;
 		i++;
@@ -162,6 +163,11 @@
 			if(u == nil){
 				fprint(fd, "/adm/users:%d: leader %s does not exist\n", lnum, f);
 				err = Enouser;
+				goto Error;
+			}
+			if(u->id == noneid){
+				fprint(fd, "/adm/users:%d: group leader may not be none\n", lnum);
+				err = Esyntax;
 				goto Error;
 			}
 			users[i].lead = u->id;
-- 
⑨