[RFC] weird use of dget_parent() in xfs_filestream_get_parent()

[RFC] weird use of dget_parent() in xfs_filestream_get_parent()

[Al Viro]

Function in question:

static struct xfs_inode *
xfs_filestream_get_parent(
        struct xfs_inode        *ip)
{
        struct inode            *inode = VFS_I(ip), *dir = NULL;
        struct dentry           *dentry, *parent;

        dentry = d_find_alias(inode);
        if (!dentry)
                goto out;

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

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

out_dput:
        dput(dentry);
out:
        return dir ? XFS_I(dir) : NULL;
}

1) dget_parent(dentry) never returns NULL; if you have IS_ROOT(dentry) you
get an equivalent of dget(dentry).  What do you want returned in that
case?

2) why bother with dget_parent() in the first place?  What's wrong with
	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
        dir = igrab(dentry->d_parent->d_inode);
	spin_unlock(&dentry->d_lock);

or, if you intended NULL for root, 
	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
	if (!IS_ROOT(dentry))
		dir = igrab(dentry->d_parent->d_inode);
	spin_unlock(&dentry->d_lock);


Is there anything subtle I'm missing here?

Re: [RFC] weird use of dget_parent() in xfs_filestream_get_parent()

[Al Viro]

On Mon, Mar 24, 2025 at 09:52:15PM +0000, Al Viro wrote:
> Function in question:
> 
> static struct xfs_inode *
> xfs_filestream_get_parent(
>         struct xfs_inode        *ip)
> {
>         struct inode            *inode = VFS_I(ip), *dir = NULL;
>         struct dentry           *dentry, *parent;
> 
>         dentry = d_find_alias(inode);
>         if (!dentry)
>                 goto out;
> 
>         parent = dget_parent(dentry);
>         if (!parent)
>                 goto out_dput;
> 
>         dir = igrab(d_inode(parent));
>         dput(parent);
> 
> out_dput:
>         dput(dentry);
> out:
>         return dir ? XFS_I(dir) : NULL;
> }
> 
> 1) dget_parent(dentry) never returns NULL; if you have IS_ROOT(dentry) you
> get an equivalent of dget(dentry).  What do you want returned in that
> case?
> 
> 2) why bother with dget_parent() in the first place?  What's wrong with
> 	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
>         dir = igrab(dentry->d_parent->d_inode);
> 	spin_unlock(&dentry->d_lock);
> 
> or, if you intended NULL for root, 
> 	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
> 	if (!IS_ROOT(dentry))
> 		dir = igrab(dentry->d_parent->d_inode);
> 	spin_unlock(&dentry->d_lock);
> 
> 
> Is there anything subtle I'm missing here?

For (2) there is - ->i_lock nests outside of ->d_lock, so igrab() under
any ->d_lock is not valid.

For (1) we provably can't get NULL - there are two returns in dget_parent():
                if (!read_seqcount_retry(&dentry->d_seq, seq))
                        return ret;
and
        spin_unlock(&ret->d_lock);
        return ret;
and neither can return a NULL - we'd oops immediately prior to getting
there.

The same goes for xrep_findparent_from_dcache() - in
        parent = dget_parent(dentry);
	if (!parent)
		goto out_dput;

	ASSERT(parent->d_sb == sc->ip->i_mount->m_super);

	pip = igrab(d_inode(parent));
	dput(parent);
we need to use dget_parent(), but we really can't get a NULL from
it.

Re: [RFC] weird use of dget_parent() in xfs_filestream_get_parent()

[Christoph Hellwig]

On Mon, Mar 24, 2025 at 09:52:15PM +0000, Al Viro wrote:
> 1) dget_parent(dentry) never returns NULL; if you have IS_ROOT(dentry) you
> get an equivalent of dget(dentry).  What do you want returned in that
> case?

xfs_filestream_get_parent is only called for regular files.  IS_ROOT can
probably only happen for partially connected trees on nfs exports, in which
case we'd want NULL;

> 
> 2) why bother with dget_parent() in the first place?  What's wrong with
> 	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
>         dir = igrab(dentry->d_parent->d_inode);
> 	spin_unlock(&dentry->d_lock);
> 
> or, if you intended NULL for root, 
> 	spin_lock(&dentry->d_lock); // stabilizes ->d_parent
> 	if (!IS_ROOT(dentry))
> 		dir = igrab(dentry->d_parent->d_inode);
> 	spin_unlock(&dentry->d_lock);
> 
> 
> Is there anything subtle I'm missing here?

Nothing really, except that this probably feelt like poking a bit too
much into internals.