Re: processes hung after sys_renameat, and 'missing' processes

[Eric W. Biederman]

Al Viro <viro@xxxxxxxxxxxxxxxxxx> writes:

> On Fri, Jun 08, 2012 at 01:36:04AM +0100, Al Viro wrote:
>> Eric, how about this - if nothing else, that makes code in there simpler
>> and less dependent on details of VFS guts:
>
> Argh.  No, it's not enough.  Why are you using ->d_iput()?  You are not
> doing anything unusual with inode; the natural place for that is in
> ->d_release() and then it will get simpler rules wrt setting ->d_fsdata.

No good reason.  We do tie inode numbers to the syfs_dirent but the
inode was changed quite a while ago to hold it's own reference
sysfs_dirent.  So using d_iput looks like sysfs historical baggage.

> As it is, you need to do that exactly after the point where you know
> that it dentry won't be dropped without going through d_add().
>
> OK, I've split that in two commits and put into vfs.git#sysfs; take a look
> and comment, please.  Should get to git.kernel.in a few...

The patches on your sysfs branch look reasonable.

I am still learly of d_materialise_unique as it allows to create alias's
on non-directories.  It isn't a functional problem as d_revalidate will
catch the issue and make it look like we have a unlink/link pair instead
of a proper rename.  However since it is possible I would like to aim
for the higher quality of implemntation and use show renames as renames.

What would be ideal for sysfs is the d_add_singleton function below.  It
does what is needed without the weird d_materialise strangeness that is
in d_materialise_unique.  But if a all singing all dancing all
confusing function is preferable I would not mind.

What I would really like is an interface so that a distrubuted/remote
filesystem can provide an inotify like stream of and we can really
implement inotify in a distributed filesystem.  But since I am too lazy
to do that I am reluctant to give up what progress I have actually made
in that direction.

Eric

diff --git a/fs/dcache.c b/fs/dcache.c
index 85c9e2b..2aab524 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2537,6 +2537,74 @@ out_nolock:
 }
 EXPORT_SYMBOL_GPL(d_materialise_unique);
 
+/**
+ * d_add_singleton - add an inode with only a single dentry
+ * @entry: dentry to instantiate
+ * @inode: inode to attach to this dentry
+ *
+ * Fill in inode information in the entry. On success, it returns NULL.
+ * If an alias of "entry" already exists, then we assume that a rename
+ * has occurred and not been reported so the alias is renamed and we
+ * return the aliased dentry and drop one reference to the inode.
+ *
+ * Note that in order to avoid conflicts with rename() etc, the caller
+ * had better be holding the parent directory semaphore.
+ *
+ * This also assumes that the inode count has been incremented
+ * (or otherwise set) by the caller to indicate that it is now
+ * in use by the dcache.
+ */
+struct dentry *d_add_singleton(struct dentry *entry, struct inode *inode)
+{
+       struct dentry *alias, *actual = entry;
+
+       if (!inode) {
+               __d_instantiate(entry, NULL);
+               d_rehash(entry);
+               goto out_nolock;
+       }
+
+       spin_lock(&inode->i_lock);
+
+       /* Does an aliased dentry already exist? */
+       alias = __d_find_alias(inode);
+       if (alias) {
+               write_seqlock(&rename_lock);
+
+               if (d_ancestor(alias, entry)) {
+                       /* Check for loops */
+                       actual = ERR_PTR(-ELOOP);
+                       spin_unlock(&inode->i_lock);
+               } else {
+                       /* Avoid aliases.  This drops inode->i_lock */
+                       actual = __d_unalias(inode, entry, alias);
+               }
+               write_sequnlock(&rename_lock);
+               if (IS_ERR(actual)) {
+                       if (PTR_ERR(actual) == -ELOOP)
+                               pr_warn_ratelimited(
+                                       "VFS: Lookup of '%s' in %s %s"
+                                       " would have caused loop\n",
+                                       entry->d_name.name,
+                                       inode->i_sb->s_type->name,
+                                       inode->i_sb->s_id);
+                       dput(alias);
+               }
+               goto out_nolock;
+       }
+       __d_instantiate(entry, inode);
+       spin_unlock(&inode->i_lock);
+       d_rehash(entry);
+out_nolock:
+       if (actual == entry ) {
+               security_d_instantiate(entry, inode);
+               return NULL;
+       }
+       iput(inode);
+       return actual;
+}
+
+
 static int prepend(char **buffer, int *buflen, const char *str, int namelen)
 {
        *buflen -= namelen;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 094789f..9613d4c 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -219,6 +219,8 @@ static inline int dname_external(struct dentry *dentry)
 extern void d_instantiate(struct dentry *, struct inode *);
 extern struct dentry * d_instantiate_unique(struct dentry *, struct inode *);
 extern struct dentry * d_materialise_unique(struct dentry *, struct inode *);
+extern struct dentry * d_materialise_unalias(struct dentry *, struct inode *);
+extern struct dentry *d_add_singleton(struct dentry *, struct inode *);
 extern void __d_drop(struct dentry *dentry);
 extern void d_drop(struct dentry *dentry);
 extern void d_delete(struct dentry *);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/