From b34779111aa1b1f320fd59b83be8f400d432e0b9 Mon Sep 17 00:00:00 2001 From: Nexoniarz Date: Mon, 10 Aug 2026 21:30:28 +0200 Subject: [PATCH] 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 --- include/defs.h | 5 ++-- include/file.h | 21 ++++++++++---- kernel/fs/fs.c | 4 +-- kernel/fs/schfs/kmount.c | 63 +++++++++++++++++++++++----------------- kernel/fs/schfs/vfs.c | 2 +- kernel/sys/sys_fs.c | 27 +++++++++++------ 6 files changed, 75 insertions(+), 47 deletions(-) diff --git a/include/defs.h b/include/defs.h index 880a741..204e57e 100644 --- a/include/defs.h +++ b/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); -uint64 schfs_ksnap_root(void); +void schfs_ksnap_unmount(uint dev); +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); diff --git a/include/file.h b/include/file.h index 5183ccb..ac669c6 100644 --- a/include/file.h +++ b/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. -// 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 -// number would make the two views alias in the inode cache and hand a reader -// whichever was cached first. -#define SCHFS_SNAP_DEV 66 +// Mounted snapshots. Same block device, an older tree root, read-only. +// +// Each 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 number would make the views alias in the inode cache and hand +// 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 diff --git a/kernel/fs/fs.c b/kernel/fs/fs.c index 7e95640..687af18 100644 --- a/kernel/fs/fs.c +++ b/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; } diff --git a/kernel/fs/schfs/kmount.c b/kernel/fs/schfs/kmount.c index 84ed4b4..beb0428 100644 --- a/kernel/fs/schfs/kmount.c +++ b/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 int ksnap_mounted; -static char *ksnapcurblk; -static char *ksnapdatablk; +static struct schfs_vol ksnapvol[SCHFS_SNAP_SLOTS]; +static int ksnap_mounted[SCHFS_SNAP_SLOTS]; +static char *ksnapcurblk[SCHFS_SNAP_SLOTS]; +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 -// first with its tree pointed at it. Nothing else differs -- same device, same +// there; what comes back is a root, and the slot's volume is a copy of the live +// 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(); - if (ksnapdatablk == 0) ksnapdatablk = kalloc(); - if (ksnapcurblk == 0 || ksnapdatablk == 0) + int slot = -1; + for (int i = 0; i < SCHFS_SNAP_SLOTS; i++) + 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.tree.root = sn.root; - ksnapvol.tree.root_crc = sn.root_crc; - ksnapvol.tree.root_gen = sn.root_generation; + ksnapvol[slot] = kvol; + ksnapvol[slot].tree.root = sn.root; + ksnapvol[slot].tree.root_crc = sn.root_crc; + 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 diff --git a/kernel/fs/schfs/vfs.c b/kernel/fs/schfs/vfs.c index 89fd7c1..475ec22 100644 --- a/kernel/fs/schfs/vfs.c +++ b/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 diff --git a/kernel/sys/sys_fs.c b/kernel/sys/sys_fs.c index c4d90e3..ea58c18 100644 --- a/kernel/sys/sys_fs.c +++ b/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) { - cprintf("schfs: snapshot mount '%s' failed (%d)\n", nm, r); + if (r < 0) { 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; }