VFS rename
authorBarret Rhoden <brho@cs.berkeley.edu>
Wed, 20 Aug 2014 18:22:31 +0000 (11:22 -0700)
committerBarret Rhoden <brho@cs.berkeley.edu>
Wed, 20 Aug 2014 18:29:53 +0000 (11:29 -0700)
Works, but it's ridiculously racy, as is most all of the VFS.  If you
have renames concurrent with other renames or much of anything, you
could bust the dentry tree or who knows what else.

The VFS (and anything that does a similar task, like a 9ns device) needs
a serious overhaul for both concurrency protection and scalability.

I also don't bother checking if new_path contains old_path.  A check
would help, but we're still in "serious redesign" territory.  Check out
my comments for more info.

That being said, you can mv files and directories around with busybox.

I didn't use testrename much, mv worked quite well.  But I'll leave it
around for now for debugging with 9ns (which needs work).  Also, glibc's
rename work, and there is no need for a separate rename() for the test
program.  Calling the syscall directly means you had an old glibc
(either not compiled right, or not loaded in KFS).

kern/include/vfs.h
kern/src/kfs.c
kern/src/syscall.c
kern/src/vfs.c
tests/testrename.c

index 5f67e79..dc39582 100644 (file)
@@ -473,6 +473,7 @@ int do_file_chmod(struct file *file, int mode);
 int do_mkdir(char *path, int mode);
 int do_rmdir(char *path);
 int do_pipe(struct file **pipe_files, int flags);
+int do_rename(char *old_path, char *new_path);
 struct file *dentry_open(struct dentry *dentry, int flags);
 void file_release(struct kref *kref);
 
index be9d470..e5bfc53 100644 (file)
@@ -456,11 +456,26 @@ int kfs_mknod(struct inode *dir, struct dentry *dentry, int mode, dev_t rdev)
        return -1;
 }
 
-/* Moves old_dentry from old_dir to new_dentry in new_dir */
-int kfs_rename(struct inode *old_dir, struct dentry *old_dentry,
-               struct inode *new_dir, struct dentry *new_dentry)
-{
-       return -1;
+/* Moves old_d from old_dir to new_d in new_dir.  TODO: super racy */
+int kfs_rename(struct inode *old_dir, struct dentry *old_d,
+               struct inode *new_dir, struct dentry *new_d)
+{
+       /* new_d is already gone, we just use it for its name.  kfs might not care
+        * about the name.  it might just use whatever the dentry says. */
+       struct kfs_i_info *old_info = (struct kfs_i_info*)old_dir->i_fs_info;
+       struct kfs_i_info *new_info = (struct kfs_i_info*)new_dir->i_fs_info;
+       printd("KFS rename: %s/%s -> %s/%s\n",
+              TAILQ_FIRST(&old_dir->i_dentry)->d_name.name, old_d->d_name.name,
+              TAILQ_FIRST(&new_dir->i_dentry)->d_name.name, new_d->d_name.name);
+       /* we want to remove from the old and add to the new.  for non-directories,
+        * we need to adjust parent's children lists (which reuses subdirs_link,
+        * yikes!).  directories aren't actually tracked by KFS; it just hopes the
+        * VFS's pinned dentry tree is enough (aka, "all paths pinned"). */
+       if (!S_ISDIR(old_d->d_inode->i_mode)) {
+               TAILQ_REMOVE(&old_info->children, old_d, d_subdirs_link);
+               TAILQ_INSERT_TAIL(&new_info->children, old_d, d_subdirs_link);
+       }
+       return 0;
 }
 
 /* Returns the char* for the symname for the given dentry.  The VFS code that
index ae17b0c..28f7833 100644 (file)
@@ -1880,7 +1880,7 @@ intreg_t sys_rename(struct proc *p, char *old_path, size_t old_path_l,
        int mountpointlen = 0;
        char *from_path = user_strdup_errno(p, old_path, old_path_l);
        char *to_path = user_strdup_errno(p, new_path, new_path_l);
-       struct chan *oldchan, *newchan = NULL;
+       struct chan *oldchan = 0, *newchan = NULL;
        int retval = -1;
 
        if ((!from_path) || (!to_path))
@@ -1888,11 +1888,18 @@ intreg_t sys_rename(struct proc *p, char *old_path, size_t old_path_l,
        printd("sys_rename :%s: to :%s: : ", from_path, to_path);
 
        /* we need a fid for the wstat. */
-       oldchan = namec(from_path, Aaccess, 0, 0);
-       if (! oldchan) {
-               printd("Could not get a chan for %s\n", from_path);
-               set_errno(ENOENT);
-               goto done;
+       /* TODO: maybe wrap the 9ns stuff better.  sysrename maybe? */
+
+       /* discard namec error */
+       if (!waserror()) {
+               oldchan = namec(from_path, Aaccess, 0, 0);
+       }
+       poperror();
+       if (!oldchan) {
+               retval = do_rename(from_path, to_path);
+               user_memdup_free(p, from_path);
+               user_memdup_free(p, to_path);
+               return retval;
        }
 
        printd("Oldchan: %C\n", oldchan);
index bc43859..8dde42f 100644 (file)
@@ -614,6 +614,23 @@ void init_sb(struct super_block *sb, struct vfsmount *vmnt,
 
 /* Dentry Functions */
 
+static void dentry_set_name(struct dentry *dentry, char *name)
+{
+       size_t name_len = strnlen(name, MAX_FILENAME_SZ);       /* not including \0! */
+       char *l_name = 0;
+       if (name_len < DNAME_INLINE_LEN) {
+               strncpy(dentry->d_iname, name, name_len);
+               dentry->d_iname[name_len] = '\0';
+               qstr_builder(dentry, 0);
+       } else {
+               l_name = kmalloc(name_len + 1, 0);
+               assert(l_name);
+               strncpy(l_name, name, name_len);
+               l_name[name_len] = '\0';
+               qstr_builder(dentry, l_name);
+       }
+}
+
 /* Helper to alloc and initialize a generic dentry.  The following needs to be
  * set still: d_op (if no parent), d_fs_info (opt), d_inode, connect the inode
  * to the dentry (and up the d_kref again), maybe dcache_put().  The inode
@@ -627,9 +644,7 @@ struct dentry *get_dentry(struct super_block *sb, struct dentry *parent,
                           char *name)
 {
        assert(name);
-       size_t name_len = strnlen(name, MAX_FILENAME_SZ);       /* not including \0! */
        struct dentry *dentry = kmem_cache_alloc(dentry_kcache, 0);
-       char *l_name = 0;
 
        if (!dentry) {
                set_errno(ENOMEM);
@@ -651,17 +666,7 @@ struct dentry *get_dentry(struct super_block *sb, struct dentry *parent,
        dentry->d_parent = parent;
        dentry->d_flags = DENTRY_USED;
        dentry->d_fs_info = 0;
-       if (name_len < DNAME_INLINE_LEN) {
-               strncpy(dentry->d_iname, name, name_len);
-               dentry->d_iname[name_len] = '\0';
-               qstr_builder(dentry, 0);
-       } else {
-               l_name = kmalloc(name_len + 1, 0);
-               assert(l_name);
-               strncpy(l_name, name, name_len);
-               l_name[name_len] = '\0';
-               qstr_builder(dentry, l_name);
-       }
+       dentry_set_name(dentry, name);
        /* Catch bugs by aggressively zeroing this (o/w we use old stuff) */
        dentry->d_inode = 0;
        return dentry;
@@ -824,6 +829,9 @@ void dcache_put(struct super_block *sb, struct dentry *key_val)
                spin_lock(&sb->s_lru_lock);
                TAILQ_REMOVE(&sb->s_lru_d, old, d_lru);
                spin_unlock(&sb->s_lru_lock);
+               /* TODO: this seems suspect.  isn't this the same memory as key_val?
+                * in which case, we just adjust the flags (remove NEG) and reinsert? */
+               assert(old != key_val); // checking TODO comment
                __dentry_free(old);
        }
        /* this returns 0 on failure (TODO: Fix this ghetto shit) */
@@ -1962,6 +1970,166 @@ error_post_dentry:
        return -1;
 }
 
+int do_rename(char *old_path, char *new_path)
+{
+       struct nameidata nd_old = {0}, *nd_o = &nd_old;
+       struct nameidata nd_new = {0}, *nd_n = &nd_new;
+       struct dentry *old_dir_d, *new_dir_d;
+       struct inode *old_dir_i, *new_dir_i;
+       struct dentry *old_d, *new_d, *unlink_d;
+       int error;
+       int retval = 0;
+       uint64_t now;
+
+       nd_o->intent = LOOKUP_ACCESS; /* maybe, might need another type */
+
+       /* get the parent, but don't follow links */
+       error = path_lookup(old_path, LOOKUP_PARENT | LOOKUP_DIRECTORY, nd_o);
+       if (error) {
+               set_errno(-error);
+               retval = -1;
+               goto out_old_path;
+       }
+       old_dir_d = nd_o->dentry;
+       old_dir_i = old_dir_d->d_inode;
+
+       old_d = do_lookup(old_dir_d, nd_o->last.name);
+       if (!old_d) {
+               set_errno(ENOENT);
+               retval = -1;
+               goto out_old_path;
+       }
+
+       nd_n->intent = LOOKUP_CREATE;
+       error = path_lookup(new_path, LOOKUP_PARENT | LOOKUP_DIRECTORY, nd_n);
+       if (error) {
+               set_errno(-error);
+               retval = -1;
+               goto out_paths_and_src;
+       }
+       new_dir_d = nd_n->dentry;
+       new_dir_i = new_dir_d->d_inode;
+       /* TODO if new_dir == old_dir, we might be able to simplify things */
+
+       if (new_dir_i->i_sb != old_dir_i->i_sb) {
+               set_errno(EXDEV);
+               retval = -1;
+               goto out_paths_and_src;
+       }
+       /* TODO: check_perms is lousy, want to just say "writable" here */
+       if (check_perms(old_dir_i, S_IWUSR) || check_perms(new_dir_i, S_IWUSR)) {
+               set_errno(EPERM);
+               retval = -1;
+               goto out_paths_and_src;
+       }
+       /* TODO: if we're doing a rename that moves a directory, we need to make
+        * sure the new_path doesn't include the old_path.  it's not as simple as
+        * just checking, since there could be a concurrent rename that breaks the
+        * check later.  e.g. what if new_dir's parent is being moved into a child
+        * of old_dir?
+        *
+        * linux has a per-fs rename mutex for these scenarios, so only one can
+        * proceed at a time.  i don't see another way to deal with it either.
+        * maybe something like flagging all dentries on the new_path with "do not
+        * move". */
+
+       /* TODO: this is all very racy.  right after we do a new_d lookup, someone
+        * else could create or unlink new_d.  need to lock here, or else push this
+        * into the sub-FS.
+        *
+        * For any locking scheme, we probably need to lock both the old and new
+        * dirs.  To prevent deadlock, we need a total ordering of all inodes (or
+        * dentries, if we locking them instead).  inode number or struct inode*
+        * will work for this. */
+       new_d = do_lookup(new_dir_d, nd_n->last.name);
+       if (new_d) {
+               if (new_d->d_inode == old_d->d_inode)
+                       goto out_paths_and_refs;        /* rename does nothing */
+               /* TODO: Here's a bunch of other racy checks we need to do, maybe in the
+                * sub-FS:
+                *
+                * if src is a dir, dst must be an empty dir if it exists (RACYx2)
+                *              racing on dst being created and it getting new entries
+                * if src is a file, dst must be a file if it exists (RACY)
+                *              racing on dst being created and still being a file
+                *              racing on dst being unlinked and a new one being added
+                */
+               /* TODO: we should allow empty dirs */
+               if (S_ISDIR(new_d->d_inode->i_mode)) {
+                       set_errno(EISDIR);
+                       retval = -1;
+                       goto out_paths_and_refs;
+               }
+               /* TODO: need this to be atomic with rename */
+               error = new_dir_i->i_op->unlink(new_dir_i, new_d);
+               if (error) {
+                       set_errno(-error);
+                       retval = -1;
+                       goto out_paths_and_refs;
+               }
+               new_d->d_flags |= DENTRY_DYING;
+               /* TODO: racy with other lookups on new_d */
+               dcache_remove(new_d->d_sb, new_d);
+               new_d->d_inode->i_nlink--;  /* TODO: race here, esp with a decref */
+               kref_put(&new_d->d_kref);
+       }
+       /* new_d is just a vessel for the name.  somewhat lousy. */
+       new_d = get_dentry(new_dir_d->d_sb, new_dir_d, nd_n->last.name);
+
+       /* TODO: more races.  need to remove old_d from the dcache, since we're
+        * about to change its parentage.  could be readded concurrently. */
+       dcache_remove(old_dir_d->d_sb, old_d);
+       error = new_dir_i->i_op->rename(old_dir_i, old_d, new_dir_i, new_d);
+       if (error) {
+               /* TODO: oh crap, we already unlinked!  now we're screwed, and violated
+                * our atomicity requirements. */
+               printk("[kernel] rename failed, you might have lost data\n");
+               set_errno(-error);
+               retval = -1;
+               goto out_paths_and_refs;
+       }
+
+       /* old_dir loses old_d, new_dir gains old_d, renamed to new_d.  this is
+        * particularly cumbersome since there are two levels here: the FS has its
+        * info about where things are, and the VFS has its dentry tree.  and it's
+        * all racy (TODO). */
+       dentry_set_name(old_d, new_d->d_name.name);
+       old_d->d_parent = new_d->d_parent;
+       if (S_ISDIR(old_d->d_inode->i_mode)) {
+               TAILQ_REMOVE(&old_dir_d->d_subdirs, old_d, d_subdirs_link);
+               old_dir_i->i_nlink--; /* TODO: racy, etc */
+               TAILQ_INSERT_TAIL(&new_dir_d->d_subdirs, old_d, d_subdirs_link);
+               new_dir_i->i_nlink--; /* TODO: racy, etc */
+       }
+
+       /* and then the third level: dcache stuff.  we could have old versions of
+        * old_d or negative versions of new_d sitting around.  dcache_put should
+        * replace a potentially negative dentry for new_d (now called old_d) */
+       dcache_put(old_dir_d->d_sb, old_d);
+
+       /* TODO could have a helper for this, but it's going away soon */
+       now = epoch_seconds();
+       old_dir_i->i_ctime.tv_sec = now;
+       old_dir_i->i_mtime.tv_sec = now;
+       old_dir_i->i_ctime.tv_nsec = 0;
+       old_dir_i->i_mtime.tv_nsec = 0;
+       new_dir_i->i_ctime.tv_sec = now;
+       new_dir_i->i_mtime.tv_sec = now;
+       new_dir_i->i_ctime.tv_nsec = 0;
+       new_dir_i->i_mtime.tv_nsec = 0;
+
+       /* fall-through */
+out_paths_and_refs:
+       kref_put(&new_d->d_kref);
+out_paths_and_src:
+       kref_put(&old_d->d_kref);
+out_paths:
+       path_release(nd_n);
+out_old_path:
+       path_release(nd_o);
+       return retval;
+}
+
 struct file *alloc_file(void)
 {
        struct file *file = kmem_cache_alloc(file_kcache, 0);
index 71804c5..b2cd520 100644 (file)
 #include <fcntl.h>
 
 
-/* Rename the file OLD to NEW.  */
-int
-rename ( const char *old, const char *new)
-{
-       syscall(123 /*SYS_rename*/, old, strlen(old), new, strlen(new), 0, 0);
-}
-
 int main(int argc, char *argv[])
 {
        int ret;