On Wed, 11 Jun 2025, Al Viro wrote:
> On Mon, Jun 09, 2025 at 05:34:12PM +1000, NeilBrown wrote:
> > After taking the directory lock (or locks) we now lock the target
> > dentries. This is pointless at present but will allow us to remove the
> > taking of the directory lock in a future patch.
> >
> > MORE WORDS
>
> Such as "why doesn't it deadlock?", presumably, seeing that you have
>
> > @@ -2003,7 +2003,14 @@ struct dentry *lookup_and_lock_hashed(struct qstr *last,
> >
> > inode_lock_nested(base->d_inode, I_MUTEX_PARENT);
> >
> > +retry:
> > dentry = lookup_one_qstr(last, base, lookup_flags);
> > + if (!IS_ERR(dentry) &&
> > + !dentry_lock(dentry, base, last, TASK_UNINTERRUPTIBLE)) {
>
> ... take dentry lock inside ->i_rwsem on parent and
>
> > bool lock_and_check_dentry(struct dentry *child, struct dentry *parent)
> > {
> > - inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
> > - if (child->d_parent == parent) {
> > - /* get the child to balance with dentry_unlock which puts it. */
> > - dget(child);
> > - return true;
> > + if (!dentry_lock(child, NULL, NULL, TASK_UNINTERRUPTIBLE))
> > + return false;
> > + if (child->d_parent != parent) {
> > + __dentry_unlock(child);
> > + return false;
> > }
> > - inode_unlock(d_inode(parent));
> > - return false;
> > + /* get the child to balance with dentry_unlock() which puts it. */
> > + dget(child);
> > + inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
>
> ... do the same in opposite order?
>
> How could that possibly work?
>
Clearly it can't - thanks for finding that.
A previous iteration has the parent locking after the dentry locking,
but I found that couldn't work. So I move the parent locking back to
the start, but must have missed this function.
That part of the patch now reads:
@@ -2143,8 +2198,8 @@ EXPORT_SYMBOL(lookup_and_lock_killable);
bool lock_and_check_dentry(struct dentry *child, struct dentry *parent)
{
inode_lock_nested(d_inode(parent), I_MUTEX_PARENT);
- if (child->d_parent == parent) {
- /* get the child to balance with dentry_unlock which puts it. */
+ if (!dentry_lock(child, parent, NULL, TASK_UNINTERRUPTIBLE)) {
+ /* get the child to balance with dentry_unlock() which puts it. */
dget(child);
return true;
}
Thanks,
NeilBrown
