diff --git a/docs/architecture/filesystem.md b/docs/architecture/filesystem.md index 2c32490..189a7ef 100644 --- a/docs/architecture/filesystem.md +++ b/docs/architecture/filesystem.md @@ -218,26 +218,57 @@ zsh: failed to write history file /root/.zsh_history.new: operation not permitte not**, and that is the honest state: these four were corrected because these four were the ones a program tripped over. +`symlink()` joined them later, and for the same reason: `symlink()` is the +classic atomic lock — create the link, and if it exists someone else holds it — +so zsh distinguishes `EEXIST` ("contended, retry") from anything else ("this +cannot work, give up"). A bare `-1` is `-EPERM`, which put every shell on the +machine in the second bucket on a filesystem that supports symlinks perfectly +well, and printed the first line above on every boot. + +### rename() replaces its target, and why it did not + +`rename()` used to refuse an existing destination, which broke +write-to-a-temporary-then-rename — how every careful program saves a file, and +how zsh saves history. The second save of anything failed with `EEXIST` and left +a `.zsh_history.new` behind for ever. + +The first attempt to fix it was reverted after it lost both files, and the note +left behind blamed HTree slot reuse. That was the wrong suspect. **`sys_rename` +was removing the old name by hand** — `memset` a `struct dirent` to zero and +`writei()` it into the parent directory at the offset `dirlookup` returned. That +is the v1 filesystem's on-disk layout, written directly, bypassing `dp->ops`. +On SchFS a directory is a B-tree with an index over it: "the record at that +offset" is not a thing, and writing zeroes there corrupted the directory while +leaving the name in the index. Adding a second removal on top of that is what +lost both files. + +Every other caller in the file already used `dirunlink()`. This one had been +open-coding a filesystem it no longer ran on, and the bug was invisible while +nothing asked it to remove two names instead of one. + +The lock discipline is the delicate part and is inherited rather than invented: +`dp_old` and `dp_new` are frequently the *same* directory reached through two +`nameiparent()` calls, which is two references to one inode. No two directory +locks are ever held at once, and the source inode stays unlocked throughout, +held alive by an `nlink` bump. + +Renaming a *directory* is still refused with `EISDIR`, and so is replacing one: +both need the `..` fixup and the ancestor-loop check that are not written. + ## What it does not do -- **`rename()` will not replace an existing target.** POSIX says it must, and - write-to-a-temporary-then-rename is how every careful program saves a file — - so the second save of anything fails with `EEXIST` where it should succeed. - It is the reason `zsh: can't rename /root/.zsh_history.new to $HISTFILE` - appears on every shell exit after the first. - - An attempt to fix it by unlinking the target first was **reverted**: the - directory is an HTree, `dirunlink` followed by `dirlink` of the same name did - not behave, and the failure mode was the worst possible one — the old file - removed and the new link then refused, losing both. Doing this properly means - understanding the HTree's slot reuse, not patching around it. - - - **No page cache.** `mmap` of a file reads the whole thing in at map time, and writable `MAP_SHARED` of a file is refused rather than faked. This is the single largest gap in the layer. - **No symlinks in procfs**, which is why `/proc//exe` is a regular file and `ps -o comm` prints `[init]`. +- **Symlinks resolve on the final path component only.** `open()`, `stat()` and + friends follow a link they are pointed at; a link in the *middle* of a path — + `/a/link/b` — is not resolved. That belongs in `namex()`, which would have to + re-enter path lookup while holding a directory lock, and getting that wrong is + a deadlock rather than a wrong answer. +- **`rename()` does not move directories**, and will not replace one. `EISDIR` + either way. - **No `AF_UNIX` socket inodes.** Socket names live in a table in `kernel/net/unix.c`, so `bind()` creates nothing that can be `stat()`ed or `unlink()`ed. diff --git a/kernel/sys/sys_fs.c b/kernel/sys/sys_fs.c index 55ad862..1bdb7f1 100644 --- a/kernel/sys/sys_fs.c +++ b/kernel/sys/sys_fs.c @@ -874,13 +874,37 @@ addr_t sys_truncate(void) return 0; } +// rename(2). +// +// Two things were wrong here and only one of them showed. +// +// **The old name was removed by hand.** `memset(&de, 0, ...)` then a writei() +// straight into the parent directory's data at the offset dirlookup() gave +// back. That is the v1 filesystem's on-disk layout -- an array of dirents -- +// written directly, bypassing dp->ops. On SchFS a directory is a B-tree with +// an HTree index over it, so zeroing "the record at that offset" wrote into a +// structure that does not work that way and left the name in the index. Every +// other caller in this file uses dirunlink(); this one had been open-coding a +// filesystem it no longer ran on. +// +// **And an existing destination was refused.** POSIX says rename() replaces +// it atomically, and the one program everybody notices this with is a shell +// saving history: zsh writes $HISTFILE.new and renames it over $HISTFILE, so +// a rename that cannot replace leaves a .zsh_history.new behind after every +// session and never updates the history at all. +// +// The lock discipline is the delicate part, and it is inherited rather than +// invented: dp_old and dp_new are frequently the *same* directory reached +// through two nameiparent() calls, which is two references to one inode. So +// no two directory locks are ever held at once -- each is locked, used, and +// unlocked before the next -- and `ip` is unlocked throughout, held alive by +// the nlink bump instead. addr_t sys_rename(void) { char *oldpath, *newpath; - struct inode *ip, *dp_old, *dp_new, *ip_src; + struct inode *ip, *dp_old, *dp_new, *ip_src, *ip_dst; char oldname[DIRSIZ], newname[DIRSIZ]; - uint off; - struct dirent de; + uint off, dstoff; int err = EPERM; if (argstr(0, &oldpath) < 0 || argstr(1, &newpath) < 0) return -EFAULT; begin_op(); @@ -898,7 +922,34 @@ addr_t sys_rename(void) if (dp_new) iput(dp_new); goto bad_rename; } + + // Clear the destination, if anything is there. ilock(dp_new); + if ((ip_dst = dirlookup(dp_new, newname, &dstoff)) != 0) { + // Both names already reach the same file. POSIX: succeed and do + // nothing -- which includes not unlinking either name, since removing + // one and adding it back is only equivalent when nothing else is + // watching. + if (ip_dst->inum == ip->inum) { + iput(ip_dst); iunlockput(dp_new); iput(dp_old); + ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); + end_op(); return 0; + } + ilock(ip_dst); + // Replacing a directory would need it to be empty and would need the + // parent's link count fixed up. Refusing is honest; the directory case + // of rename is unimplemented as a whole. + if (ip_dst->type == T_DIR) { + iunlockput(ip_dst); iunlockput(dp_new); iput(dp_old); + err = EISDIR; goto bad_rename; + } + if (dirunlink(dp_new, newname, dstoff) < 0) { + iunlockput(ip_dst); iunlockput(dp_new); iput(dp_old); + err = EIO; goto bad_rename; + } + ip_dst->nlink--; iupdate(ip_dst); iunlockput(ip_dst); + } + if (dirlink(dp_new, newname, ip->inum) < 0) { iunlockput(dp_new); iput(dp_old); err = EEXIST; goto bad_rename; } @@ -909,8 +960,10 @@ addr_t sys_rename(void) end_op(); return -ENOENT; } iput(ip_src); - memset(&de, 0, sizeof(de)); - if (writei(dp_old, 0, (uint64)&de, off, sizeof(de)) != sizeof(de)) panic("rename: writei"); + // Through the filesystem, not through writei(). See the note above. + if (dirunlink(dp_old, oldname, off) < 0) { + iunlockput(dp_old); err = EIO; goto bad_rename; + } iunlockput(dp_old); ilock(ip); ip->nlink--; iupdate(ip); iunlockput(ip); end_op(); return 0;