schfs: several snapshots at once, and unmount gives the slot back

Two loose ends from the mount-by-name commit, both closed.

A slot is a device number, so supporting more than one snapshot is a
small array rather than a redesign: SCHFS_SNAP_DEV through
SCHFS_SNAP_DEV + SCHFS_SNAP_SLOTS - 1, each with its own volume and its
own scratch pages, allocated on first use. Four of them -- "one at a
time" is an odd thing to have to explain, and the cost is two pages per
slot. Not many, because each one is a device number and those are a
small fixed space.

The leak mattered more. umount removed the mount table entry and left the
slot marked in use, so a snapshot could be unmounted and then never
mounted again until reboot. sys_umount2 releases it now, keyed off the
device it already has in hand.

Verified in the running system: two snapshots mounted at once, both
readable, both unmounted, and a third mount afterwards succeeds and reads
correctly -- which is the case that failed before.

schfs-check passes; bootcheck 108 [ok].

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

include/defs.h

@@ -180,9 +180,10 @@ uint64          schfs_kroot(void);
 // The read path takes a device number so the VFS can address either view; there
 // is no write path, because a snapshot that could be written to would not be
 // one. See kernel/fs/schfs/kmount.c.
+// Returns the device number of the slot the snapshot landed in, or negative.
 int             schfs_ksnap_mount(const char *name, unsigned namelen);
-void            schfs_ksnap_unmount(void);
+void            schfs_ksnap_unmount(uint dev);
-uint64          schfs_ksnap_root(void);
+uint64          schfs_ksnap_root(uint dev);
 long            schfs_kread_dev(uint dev, uint64 ino, uint64 off, void *buf,
                     unsigned n);
 int             schfs_kstat_dev(uint dev, uint64 ino, struct schfs_inode *out);

include/file.h

@@ -102,12 +102,21 @@ struct inode {
 
 // Device number that means "the synthetic /proc". Like SCHFS_DEV, it is
 // deliberately nowhere near a real disk's 0 or 1.
-// A mounted snapshot. Same block device, a different tree root, read-only.
+// Mounted snapshots. Same block device, an older tree root, read-only.
-// It needs its own device number because an inode is identified by (dev, inum)
+//
-// and a snapshot's object ids are the *same numbers* as the live tree's -- one
+// Each needs its own device number because an inode is identified by
-// number would make the two views alias in the inode cache and hand a reader
+// (dev, inum) and a snapshot's object ids are the *same numbers* as the live
-// whichever was cached first.
+// tree's -- one number would make the views alias in the inode cache and hand
-#define SCHFS_SNAP_DEV 66
+// a reader whichever was cached first. So a slot is a device: SCHFS_SNAP_DEV
+// through SCHFS_SNAP_DEV + SCHFS_SNAP_SLOTS - 1.
+//
+// Four rather than one because "one at a time" is an odd thing to explain and
+// the cost is two scratch pages per slot, allocated on first use. Four rather
+// than many because each is a device number and they are a fixed, small space.
+#define SCHFS_SNAP_SLOTS 4
+#define SCHFS_SNAP_DEV   66
+#define SCHFS_IS_SNAP_DEV(d) \
+    ((d) >= SCHFS_SNAP_DEV && (d) < SCHFS_SNAP_DEV + SCHFS_SNAP_SLOTS)
 
 #define PROCFS_DEV 65
 

kernel/fs/fs.c

@@ -284,7 +284,7 @@ extern const struct fs_ops procfs_ops;
 int
 fs_readonly(uint dev)
 {
-  return dev == SCHFS_SNAP_DEV;
+  return SCHFS_IS_SNAP_DEV(dev);
 }
 
 static const struct fs_ops *
@@ -293,7 +293,7 @@ ops_for(uint dev)
   if (dev == SCHFS_DEV)  return &schfs_ops;
   // A mounted snapshot is the same filesystem seen through an older root, so it
   // is the same operations; they dispatch on the device themselves.
-  if (dev == SCHFS_SNAP_DEV) return &schfs_ops;
+  if (SCHFS_IS_SNAP_DEV(dev)) return &schfs_ops;
   if (dev == PROCFS_DEV) return &procfs_ops;
   return &v1_ops;
 }

kernel/fs/schfs/kmount.c

@@ -58,10 +58,10 @@ static int               kmounted;
 // A second volume rather than a parameter threaded through all 23 entry points,
 // because only the read side can ever address it -- a snapshot cannot be
 // written to, so every mutation keeps talking to kvol and needs no change.
-static struct schfs_vol  ksnapvol;
+static struct schfs_vol  ksnapvol[SCHFS_SNAP_SLOTS];
-static int               ksnap_mounted;
+static int               ksnap_mounted[SCHFS_SNAP_SLOTS];
-static char             *ksnapcurblk;
+static char             *ksnapcurblk[SCHFS_SNAP_SLOTS];
-static char             *ksnapdatablk;
+static char             *ksnapdatablk[SCHFS_SNAP_SLOTS];
 
 // Block buffers, allocated once at mount.
 //
@@ -267,44 +267,49 @@ schfs_kmount(uint dev)
 static struct schfs_vol *
 vol_for(uint dev)
 {
-    return dev == SCHFS_SNAP_DEV ? &ksnapvol : &kvol;
+    return SCHFS_IS_SNAP_DEV(dev) ? &ksnapvol[dev - SCHFS_SNAP_DEV] : &kvol;
 }
 
 static char *
 curblk_for(uint dev)
 {
-    return dev == SCHFS_SNAP_DEV ? ksnapcurblk : kcurblk;
+    return SCHFS_IS_SNAP_DEV(dev) ? ksnapcurblk[dev - SCHFS_SNAP_DEV] : kcurblk;
 }
 
 static char *
 datablk_for(uint dev)
 {
-    return dev == SCHFS_SNAP_DEV ? ksnapdatablk : kdatablk;
+    return SCHFS_IS_SNAP_DEV(dev) ? ksnapdatablk[dev - SCHFS_SNAP_DEV] : kdatablk;
 }
 
 static int
 mounted_for(uint dev)
 {
-    return dev == SCHFS_SNAP_DEV ? ksnap_mounted : kmounted;
+    return SCHFS_IS_SNAP_DEV(dev) ? ksnap_mounted[dev - SCHFS_SNAP_DEV] : kmounted;
 }
 
-// Mount a snapshot by name, read-only.
+// Mount a snapshot by name, read-only. Returns the device number of the slot
+// it landed in, or a negative error.
 //
 // The snapshot's records live in the *live* tree, so the name is resolved
-// there; what comes back is a root, and the second volume is a copy of the
+// there; what comes back is a root, and the slot's volume is a copy of the live
-// first with its tree pointed at it. Nothing else differs -- same device, same
+// one with its tree pointed at it. Nothing else differs -- same device, same
 // node format, same checksums.
 int
 schfs_ksnap_mount(const char *name, unsigned namelen)
 {
   if (!kmounted)
     return -1;
-  if (ksnap_mounted)
-    return -2;                    /* one at a time, for now */
 
-  if (ksnapcurblk == 0)  ksnapcurblk  = kalloc();
+  int slot = -1;
-  if (ksnapdatablk == 0) ksnapdatablk = kalloc();
+  for (int i = 0; i < SCHFS_SNAP_SLOTS; i++)
-  if (ksnapcurblk == 0 || ksnapdatablk == 0)
+    if (!ksnap_mounted[i]) { slot = i; break; }
+  if (slot < 0)
+    return -2;                    /* every slot in use */
+
+  if (ksnapcurblk[slot] == 0)  ksnapcurblk[slot]  = kalloc();
+  if (ksnapdatablk[slot] == 0) ksnapdatablk[slot] = kalloc();
+  if (ksnapcurblk[slot] == 0 || ksnapdatablk[slot] == 0)
     return -3;
 
   acquiresleep(&schfs_lock);
@@ -349,31 +354,35 @@ schfs_ksnap_mount(const char *name, unsigned namelen)
   // Same volume, older root. The device, the space map and the block callbacks
   // are all the live volume's -- a snapshot is not a separate filesystem, it is
   // a second way into this one.
-  ksnapvol = kvol;
+  ksnapvol[slot]               = kvol;
-  ksnapvol.tree.root     = sn.root;
+  ksnapvol[slot].tree.root     = sn.root;
-  ksnapvol.tree.root_crc = sn.root_crc;
+  ksnapvol[slot].tree.root_crc = sn.root_crc;
-  ksnapvol.tree.root_gen = sn.root_generation;
+  ksnapvol[slot].tree.root_gen = sn.root_generation;
   // Its own scratch, or a read through the snapshot would tread on a cursor
   // the live volume still has positioned.
-  ksnapvol.ctx.a = ksnapcurblk;
+  ksnapvol[slot].ctx.a = ksnapcurblk[slot];
 
-  ksnap_mounted = 1;
+  ksnap_mounted[slot] = 1;
   releasesleep(&schfs_lock);
-  return 0;
+  return SCHFS_SNAP_DEV + slot;
 }
 
 void
-schfs_ksnap_unmount(void)
+schfs_ksnap_unmount(uint dev)
 {
+  if (!SCHFS_IS_SNAP_DEV(dev))
+    return;
   acquiresleep(&schfs_lock);
-  ksnap_mounted = 0;
+  ksnap_mounted[dev - SCHFS_SNAP_DEV] = 0;
   releasesleep(&schfs_lock);
 }
 
 uint64
-schfs_ksnap_root(void)
+schfs_ksnap_root(uint dev)
 {
-  return ksnap_mounted ? ksnapvol.root_objectid : 0;
+  if (!SCHFS_IS_SNAP_DEV(dev) || !ksnap_mounted[dev - SCHFS_SNAP_DEV])
+    return 0;
+  return ksnapvol[dev - SCHFS_SNAP_DEV].root_objectid;
 }
 
 // The read path, addressable per device so a snapshot can be read through the

kernel/fs/schfs/vfs.c

@@ -109,7 +109,7 @@ schfs_vfs_iload(struct inode *ip)
 static int
 readonly(const struct inode *ip)
 {
-  return ip->dev == SCHFS_SNAP_DEV;
+  return SCHFS_IS_SNAP_DEV(ip->dev);
 }
 
 static void

kernel/sys/sys_fs.c

@@ -1417,7 +1417,8 @@ addr_t sys_mount(void)
     // the first second thing to mount. `@` rather than a path because a
     // snapshot is not a device: there is no node in /dev for it and there
     // should not be, since it lives inside the volume that is already mounted.
-    int snapmount = 0;
+    int  snapmount = 0;
+    uint snapdev   = 0;
     if (source && source[0] == '@') {
         const char *nm = source + 1;
         unsigned nlen = 0;
@@ -1429,24 +1430,24 @@ addr_t sys_mount(void)
         // Distinct codes, because "resource busy" for every possible failure
         // tells the caller nothing and cost an hour of guessing once already.
         int r = schfs_ksnap_mount(nm, nlen);
-        if (r != 0) {
+        if (r < 0) {
-            cprintf("schfs: snapshot mount '%s' failed (%d)\n", nm, r);
             switch (r) {
             case -1: return -ENODEV;   /* no volume mounted to look in */
-            case -2: return -EBUSY;    /* a snapshot is already mounted */
+            case -2: return -EBUSY;    /* every snapshot slot in use */
             case -3: return -ENOMEM;   /* no scratch blocks */
             default: return -ENOENT;   /* no snapshot by that name */
             }
         }
+        snapdev  = (uint)r;
         snapmount = 1;
     }
     if (!schfs_kmounted())
         return -ENODEV;
 
-    uint64 root = snapmount ? schfs_ksnap_root() : schfs_kroot();
+    uint64 root = snapmount ? schfs_ksnap_root(snapdev) : schfs_kroot();
     if (root == 0) {
         if (snapmount)
-            schfs_ksnap_unmount();
+            schfs_ksnap_unmount(snapdev);
         return -ENODEV;
     }
 
@@ -1468,12 +1469,12 @@ addr_t sys_mount(void)
     // the only volume there could be. A snapshot is a second device, and its
     // mount point is necessarily on the live root -- there is nowhere else to
     // put it -- so the old spelling refused every snapshot mount.
-    uint fsdev = snapmount ? SCHFS_SNAP_DEV : SCHFS_DEV;
+    uint fsdev = snapmount ? snapdev : SCHFS_DEV;
     if (dev == fsdev)
         return -EBUSY;
     if (mount_add(dev, inum, fsdev, root) != 0) {
         if (snapmount)
-            schfs_ksnap_unmount();
+            schfs_ksnap_unmount(snapdev);
         return -EBUSY;
     }
     return 0;
@@ -1495,5 +1496,13 @@ addr_t sys_umount2(void)
     iput(dp);
     end_op();
 
-    return mount_del_by_root(dev, inum) == 0 ? 0 : -EINVAL;
+    if (mount_del_by_root(dev, inum) != 0)
+        return -EINVAL;
+
+    // Release the snapshot slot with the mount. Without this the table entry
+    // went away and the slot stayed taken, so a volume could be unmounted and
+    // never mounted again until reboot.
+    if (SCHFS_IS_SNAP_DEV(dev))
+        schfs_ksnap_unmount(dev);
+    return 0;
 }