On Tue, Jul 16, 2013 at 06:14:58PM +0200, Miklos Szeredi wrote:
> On Tue, Jul 16, 2013 at 09:15:16AM -0400, Brian Foster wrote:
>
> > I'm not sure why it would need to have a valid inode. A dentry with a
> > NULL inode is valid, no?
>
> It is valid, yes. It's called a "negative" dentry, which caches the
> information
> that the file does not exist.
>
> > I think the question is whether the above state (multiple, hashed dentries)
> > can be valid for whatever reason. It certainly looks suspicious.
>
> That state is not valid. So we certainly need to unhash the negative dentry
> first, before hashing a new one. We could also reuse the old dentry, but that
> is just an optimization.
>
> Below patch fixes several issues with that function. Could you please give
> it a
> go?
Thanks Miklos! This looks much cleaner and the additional error checking
surely reduces the chance of any further problems. I've added only one
little comment in the patch below.
I can confirm that your patch fixes the original panic/BUG() with both
Brian's test-case and the GlusterFS regression tests.
Feel free to add my Tested-by/Reviewed-by signoff for this patch.
Cheers,
Niels
>
> Thanks,
> Miklos
>
>
> diff --git a/fs/fuse/dir.c b/fs/fuse/dir.c
> index 0eda527..a8208c5 100644
> --- a/fs/fuse/dir.c
> +++ b/fs/fuse/dir.c
> @@ -1223,28 +1223,45 @@ static int fuse_direntplus_link(struct file *file,
> if (name.name[1] == '.' && name.len == 2)
> return 0;
> }
> +
> + if (invalid_nodeid(o->nodeid))
> + return -EIO;
> + if (!fuse_valid_type(o->attr.mode))
> + return -EIO;
> +
> fc = get_fuse_conn(dir);
>
> name.hash = full_name_hash(name.name, name.len);
> dentry = d_lookup(parent, &name);
> - if (dentry && dentry->d_inode) {
> + if (dentry) {
> inode = dentry->d_inode;
> - if (get_node_id(inode) == o->nodeid) {
> + if (!inode) {
> + d_drop(dentry);
> + } else if (get_node_id(inode) != o->nodeid ||
> + ((o->attr.mode ^ inode->i_mode) & S_IFMT)) {
> + err = d_invalidate(dentry);
> + if (err)
> + goto out;
> + } else if (is_bad_inode(inode)) {
> + err = -EIO;
> + goto out;
> + } else {
> struct fuse_inode *fi;
> fi = get_fuse_inode(inode);
> spin_lock(&fc->lock);
> fi->nlookup++;
> spin_unlock(&fc->lock);
>
> + fuse_change_attributes(inode, &o->attr,
> + entry_attr_timeout(o),
> + attr_version);
> +
> /*
> * The other branch to 'found' comes via fuse_iget()
> * which bumps nlookup inside
> */
> goto found;
> }
> - err = d_invalidate(dentry);
> - if (err)
> - goto out;
> dput(dentry);
> dentry = NULL;
You might want to remove that 'dentry = NULL' line, as the result of
d_alloc() is assigned to dentry on line below the if-statement.
> }
> @@ -1259,25 +1276,30 @@ static int fuse_direntplus_link(struct file *file,
> if (!inode)
> goto out;
>
> - alias = d_materialise_unique(dentry, inode);
> - err = PTR_ERR(alias);
> - if (IS_ERR(alias))
> - goto out;
> + if (S_ISDIR(inode->i_mode)) {
> + mutex_lock(&fc->inst_mutex);
> + alias = fuse_d_add_directory(dentry, inode);
> + mutex_unlock(&fc->inst_mutex);
> + err = PTR_ERR(alias);
> + if (IS_ERR(alias)) {
> + iput(inode);
> + goto out;
> + }
> + } else {
> + alias = d_splice_alias(inode, dentry);
> + }
> +
> if (alias) {
> dput(dentry);
> dentry = alias;
> }
>
> found:
> - fuse_change_attributes(inode, &o->attr, entry_attr_timeout(o),
> - attr_version);
> -
> fuse_change_entry_timeout(dentry, o);
>
> err = 0;
> out:
> - if (dentry)
> - dput(dentry);
> + dput(dentry);
> return err;
> }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
