Skip to contentSkip to conversation

new helper: d_obtain_alias

PATCH In review Christoph Hellwig 3 participants 4 messages Aug 11, 2008 – Sep 11, 2008 VFS

lore
Share & tools
lore

Christoph Hellwig

author
#0 lore raw

PATCH 2/4 new helper: d_obtain_alias

The calling conventions of d_alloc_anon are rather unfortunate for all users, and it's name is not very descriptive either.

Add d_obtain_alias as a new exported helper that drops the inode reference in the failure case, too and allows to pass-through NULL pointers and inodes to allow for tail-calls in the export operations.

Incidentally this helper already existed as a private function in libfs.c as exportfs_d_alloc so kill that one and switch the callers to d_obtain_alias.

Signed-off-by Christoph Hellwig <[email protected]>
fs/dcache.c +35 -0
Index: linux-2.6/fs/dcache.c
===================================================================
@@ -1174,6 +1174,41 @@ struct dentry * d_alloc_anon(struct inod
 	return res;
 }
 
+/**
+ * d_obtain_alias - find or allocate a dentry for a given inode
+ * @inode: inode to allocate the dentry for
+ *
+ * Obtain a dentry for an inode resulting from NFS filehandle conversion or
+ * similar open by handle operations.  The returned dentry may be anonymous,
+ * or may have a full name (if the inode was already in the cache).
+ *
+ * When called on a directory inode, we must ensure that the inode only ever
+ * has one dentry.  If a dentry is found, that is returned instead of
+ * allocating a new one.
+ *
+ * On successful return, the reference to the inode has been transferred
+ * to the dentry.  If %NULL is returned (indicating kmalloc failure),
+ * the reference on the inode has been released.  To make it easier
+ * to use in export operations a NULL or IS_ERR inode may be passed in
+ * and will be casted to the corresponding NULL or IS_ERR dentry.
+ */
+struct dentry *d_obtain_alias(struct inode *inode)
+{
+	struct dentry *dentry;
+
+	if (!inode)
+		return NULL;
+	if (IS_ERR(inode))
+		return ERR_CAST(inode);
+
+	dentry = d_alloc_anon(inode);
+	if (!dentry) {
+		iput(inode);
+		dentry = ERR_PTR(-ENOMEM);
+	}
+	return dentry;
+}
+EXPORT_SYMBOL_GPL(d_obtain_alias);
 
 /**
  * d_splice_alias - splice a disconnected dentry into the tree if one exists
fs/libfs.c +2 -24
Index: linux-2.6/fs/libfs.c
===================================================================
@@ -732,28 +732,6 @@ out:
 	return ret;
 }
 
-/*
- * This is what d_alloc_anon should have been.  Once the exportfs
- * argument transition has been finished I will update d_alloc_anon
- * to this prototype and this wrapper will go away.   --hch
- */
-static struct dentry *exportfs_d_alloc(struct inode *inode)
-{
-	struct dentry *dentry;
-
-	if (!inode)
-		return NULL;
-	if (IS_ERR(inode))
-		return ERR_PTR(PTR_ERR(inode));
-
-	dentry = d_alloc_anon(inode);
-	if (!dentry) {
-		iput(inode);
-		dentry = ERR_PTR(-ENOMEM);
-	}
-	return dentry;
-}
-
 static int fileid_length(int fileid_type)
 {
 	switch (fileid_type) {

@@ -812,7 +790,7 @@ struct dentry *generic_fh_to_dentry(stru
 		break;
 	}
 
-	return exportfs_d_alloc(inode);
+	return d_obtain_alias(inode);
 }
 EXPORT_SYMBOL_GPL(generic_fh_to_dentry);
 

@@ -845,7 +823,7 @@ struct dentry *generic_fh_to_parent(stru
 		break;
 	}
 
-	return exportfs_d_alloc(inode);
+	return d_obtain_alias(inode);
 }
 EXPORT_SYMBOL_GPL(generic_fh_to_parent);
include/linux/dcache.h +1 -0
Index: linux-2.6/include/linux/dcache.h
===================================================================
@@ -231,6 +231,7 @@ extern struct dentry * d_alloc(struct de
 extern struct dentry * d_alloc_anon(struct inode *);
 extern struct dentry * d_splice_alias(struct inode *, struct dentry *);
 extern struct dentry * d_add_ci(struct dentry *, struct inode *, struct qstr *);
+extern struct dentry * d_obtain_alias(struct inode *);
 extern void shrink_dcache_sb(struct super_block *);
 extern void shrink_dcache_parent(struct dentry *);
 extern void shrink_dcache_for_umount(struct super_block *);
#1 lore raw
Quoted 6 lines · Christoph Hellwig

+/**
+ * d_obtain_alias - find or allocate a dentry for a given inode
+ * @inode: inode to allocate the dentry for
+ *
+ * Obtain a dentry for an inode resulting from NFS filehandle conversion or
+ * similar open by handle operations. The returned dentry may be anonymous,

Quoted 8 lines

+ * or may have a full name (if the inode was already in the cache).
+ *
+ * When called on a directory inode, we must ensure that the inode only ever
+ * has one dentry. If a dentry is found, that is returned instead of
+ * allocating a new one.
+ *
+ * On successful return, the reference to the inode has been transferred
+ * to the dentry. If %NULL is returned (indicating kmalloc failure),

                         ^^^^^
                              ERR_PTR(-ENOMEM)
Quoted 1 line

+ * the reference on the inode has been released.

That's useful.

Quoted 4 lines

To make it easier
+ * to use in export operations a NULL or IS_ERR inode may be passed in
+ * and will be casted to the corresponding NULL or IS_ERR dentry.
+ */

Mixing NULL with ERR_PTR() seems like a bad idea. It should be either one or the other but not both.

Miklos

#2 lore raw
Quoted 5 lines · Miklos Szeredi
Quoted 2 lines

+ * and will be casted to the corresponding NULL or IS_ERR dentry.
+ */

Mixing NULL with ERR_PTR() seems like a bad idea. It should be either one or the other but not both.

It's how the existing interface works. I think we should move to only ERR_PTR returns, but it's not very high on my todo list.

#3 lore raw
Quoted 8 lines · Christoph Hellwig
Quoted 5 lines · Miklos Szeredi
Quoted 2 lines

+ * and will be casted to the corresponding NULL or IS_ERR dentry.
+ */

Mixing NULL with ERR_PTR() seems like a bad idea. It should be either one or the other but not both.

It's how the existing interface works.

If you mean ->fh_to_dentry() and ->fh_to_parent(), yes they do work this way. But a NULL return is simply equivalent to a -ESTALE return, so why not turn !inode inot an -ESTALE in d_obtain_alias()? That would fix the interface and the bug in fuse.

Miklos