Btrfs devel - May 2008 - Drop dcache entry after creating snapshot and subvolume

Hi all,

when creating a new snapshot or subvolume we need to drop an existing 
negative dentry for the new name, else this happens:

host ~ # ls -l /mnt/btrfs
total 1
dr-xr-xr-x 1 root root 0 Jan  1  1970 default
host ~ # stat /mnt/btrfs/snapshot
stat: cannot stat `/mnt/btrfs/snapshot'': No such file or directory
host ~ # btrfsctl -s snapshot /mnt/btrfs/default
ioctl returns 0
host ~ # stat /mnt/btrfs/snapshot
stat: cannot stat `/mnt/btrfs/snapshot'': No such file or directory
host ~ # ls -l /mnt/btrfs
ls: cannot access /mnt/btrfs/snapshot: No such file or directory
total 1
dr-xr-xr-x 1 root root 0 Jan  1  1970 default
d????????? ? ?    ?    ?            ? snapshot

I have a patch (see below) that does an explicit d_drop() on the dentry 
after looking it up via d_find_alias() and d_lookup(), starting at the 
root inode. Currently it''s for snapshot creation only, subvolume
creation
needs the same. There doesn''t seem to be a kernel function for this
case
and using the normal d_revalidate method is inefficient as snapshot and 
subvolume creation is the only place where we need this and the creation 
is a rare case. Is this the right way to go?

Sven

---
  transaction.c |   18 ++++++++++++++++++
  1 file changed, 18 insertions(+)

--- a/transaction.c
+++ b/transaction.c
@@ -557,6 +557,8 @@
  	struct btrfs_root *tree_root = fs_info->tree_root;
  	struct btrfs_root *root = pending->root;
  	struct extent_buffer *tmp;
+	struct dentry *alias, *snap;
+	struct qstr qstr;
  	int ret;
  	u64 objectid;

@@ -604,6 +606,22 @@
  	ret = btrfs_insert_inode_ref(trans, root->fs_info->tree_root,
  			     pending->name, strlen(pending->name), objectid,
  			     root->fs_info->sb->s_root->d_inode->i_ino);
+
+	/*
+	 * We need to drop the dcache entry for the new snapshot.
+	 */
+	alias = d_find_alias(root->fs_info->sb->s_root->d_inode);
+	if (alias) {
+		qstr.name = pending->name;
+		qstr.len = strlen(qstr.name);
+		qstr.hash = full_name_hash(qstr.name, qstr.len);
+		snap = d_lookup(alias, &qstr);
+		dput(alias);
+		if (snap) {
+			d_drop(snap);
+			dput(snap);
+		}
+	}
  fail:
  	kfree(new_root_item);
  	return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, May 28, 2008 at 12:41:24AM +0200, Sven Wegener
wrote:
> I have a patch (see below) that does an explicit d_drop() on the dentry 
> after looking it up via d_find_alias() and d_lookup(), starting at the root
> inode. Currently it''s for snapshot creation only, subvolume
creation needs
> the same. There doesn''t seem to be a kernel function for this case
and
> using the normal d_revalidate method is inefficient as snapshot and 
> subvolume creation is the only place where we need this and the creation is
> a rare case. Is this the right way to go?
I don''t like this manual dropping very much.  Rather the snapshot
should
be created using vfs_mkdir or equivalent opencoded bits that actually
turn the negative dentry into the real instanciated one.

Then again it might actually be better to have a separate superblock
for the snapshot to not get a too messy dentry tree, but before
commenting on that I need to actually dig into that area of btrfs again.
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wednesday 28 May 2008, Christoph Hellwig wrote:
> On Wed, May 28, 2008 at 12:41:24AM +0200, Sven Wegener wrote:
> > I have a patch (see below) that does an explicit d_drop() on the
dentry
> > after looking it up via d_find_alias() and d_lookup(), starting at the
> > root inode. Currently it''s for snapshot creation only,
subvolume creation
> > needs the same. There doesn''t seem to be a kernel function
for this case
> > and using the normal d_revalidate method is inefficient as snapshot
and
> > subvolume creation is the only place where we need this and the
creation
> > is a rare case. Is this the right way to go?
>
> I don''t like this manual dropping very much.  Rather the snapshot
should
> be created using vfs_mkdir or equivalent opencoded bits that actually
> turn the negative dentry into the real instanciated one.
>
> Then again it might actually be better to have a separate superblock
> for the snapshot to not get a too messy dentry tree, but before
> commenting on that I need to actually dig into that area of btrfs again.
There''s a looming dentry mess with snapshot deletion, where I want to
be able
to delete a snapshot or subvolume by efficiently walking the btree, but I 
first need to find any dentries (with the same semantics as unmount).

But, I don''t think it makes sense to give each subvolume its own super,
there''s too much shared between them (even btree blocks).  Also, today
all
the subvolumes and snapshots live in the root directory, but the long term 
setup will have a real directory tree there, and so we need to make directory 
operations for that btree.

But for today, I think Sven''s patch is a good idea.

-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

We need to invalidate an existing dcache entry after creating a new 
snapshot or subvolume, because a negative dache entry will stop us from 
accessing the new snapshot or subvolume.

---
  ctree.h       |   23 +++++++++++++++++++++++
  inode.c       |    4 ++++
  transaction.c |    4 ++++
  3 files changed, 31 insertions(+)

diff -r 4b7e2b315a32 ctree.h
--- a/ctree.h	Thu May 29 10:31:43 2008 -0400
+++ b/ctree.h	Sat May 31 19:10:37 2008 +0000
@@ -1326,6 +1326,29 @@
  #endif
  }

+/*
+ * Invalidate a single dcache entry at the root of the filesystem.
+ * Needed after creation of snapshot or subvolume.
+ */
+
+static inline void invalidate_dcache_root(struct btrfs_root *root, char *name)
{
+	struct dentry *alias, *entry;
+	struct qstr qstr;
+
+	alias = d_find_alias(root->fs_info->sb->s_root->d_inode);
+	if (alias) {
+		qstr.name = name;
+		qstr.len = strlen(qstr.name);
+		qstr.hash = full_name_hash(qstr.name, qstr.len);
+		entry = d_lookup(alias, &qstr);
+		dput(alias);
+		if (entry) {
+			d_invalidate(entry);
+			dput(entry);
+		}
+	}
+}
+
  /* extent-tree.c */
  u32 btrfs_count_snapshots_in_path(struct btrfs_root *root,
  				  struct btrfs_path *count_path,
diff -r 4b7e2b315a32 inode.c
--- a/inode.c	Thu May 29 10:31:43 2008 -0400
+++ b/inode.c	Sat May 31 19:10:37 2008 +0000
@@ -2761,6 +2761,10 @@
  	ret = btrfs_update_inode(trans, new_root, inode);
  	if (ret)
  		goto fail;
+
+	/* Invalidate existing dcache entry for new subvolume. */
+	invalidate_dcache_root(root, name);
+
  fail:
  	nr = trans->blocks_used;
  	err = btrfs_commit_transaction(trans, new_root);
diff -r 4b7e2b315a32 transaction.c
--- a/transaction.c	Thu May 29 10:31:43 2008 -0400
+++ b/transaction.c	Sat May 31 19:10:37 2008 +0000
@@ -604,6 +604,10 @@
  	ret = btrfs_insert_inode_ref(trans, root->fs_info->tree_root,
  			     pending->name, strlen(pending->name), objectid,
  			     root->fs_info->sb->s_root->d_inode->i_ino);
+
+	/* Invalidate existing dcache entry for new snapshot. */
+	invalidate_dcache_root(root, pending->name);
+
  fail:
  	kfree(new_root_item);
  	return ret;
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html