[RFC] dget_parent() misuse in xfs_filestream_get_parent()

[RFC] dget_parent() misuse in xfs_filestream_get_parent()

[Al Viro]

	dget_parent() never returns NULL.  So this

        parent = dget_parent(dentry);
        if (!parent)
                goto out_dput;

        dir = igrab(d_inode(parent));
        dput(parent);

out_dput:

is obviously fishy.  What is that code trying to do?  Is that
"dentry might be a root of disconnected tree, in which case
we want xfs_filestream_get_parent() to return NULL"?  If so,
that should be

        parent = dget_parent(dentry);
        if (parent != dentry)
		dir = igrab(d_inode(parent));
        dput(parent);

If we want dentry itself, that dir = igrab(...) should be
unconditional.  I'm not familiar enough with that code,
and I'd rather not go into YAMassiveSideRTFS, so...

Re: [RFC] dget_parent() misuse in xfs_filestream_get_parent()

[Dave Chinner]

On Fri, Jun 28, 2019 at 07:00:27AM +0100, Al Viro wrote:
> 	dget_parent() never returns NULL.  So this
> 
>         parent = dget_parent(dentry);
>         if (!parent)
>                 goto out_dput;
> 
>         dir = igrab(d_inode(parent));
>         dput(parent);
> 
> out_dput:
> 
> is obviously fishy.  What is that code trying to do?  Is that
> "dentry might be a root of disconnected tree, in which case
> we want xfs_filestream_get_parent() to return NULL"? 

We want the parent inode of the current file inode if it is in
memory. We don't care about the parent dentry that is returned as
such, it's just the mechanism for finding the directory inode. The
directory inode is what holds the allocation policy for all files in
that directory, and that's what we need here.

If there is no parent directory inode in memory, then we'll just use
the default allocator behaviour rather than the context specific
one we get from the directory inode...

> If so,
> that should be
> 
>         parent = dget_parent(dentry);
>         if (parent != dentry)
> 		dir = igrab(d_inode(parent));
>         dput(parent);

Seems reasonable. This code is largely legacy functionality, the
allocator was really a specific workload policy that was never
widely deployed and so the combination of disconnected dentries and
this allocator have probably never been seen in real life...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com