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.
