Re: [PATCH 5/8] Introduce S_DYING which warns that S_DEAD might follow.

[Al Viro <viro@zeniv.linux.org.uk>]

On Mon, Jun 09, 2025 at 05:34:10PM +1000, NeilBrown wrote:
> Once we support directory operations (e.g. create) without requiring the
> parent to be locked, the current practice locking a directory while
> processing rmdir() or similar will not be sufficient to wait for
> operations to complete and to block further operations.
> 
> This patch introduced a new inode flag S_DYING.  It indicates that
> a rmdir or similar is being processed and new directory operations must
> not commence in the directory.  They should not abort either as the
> rmdir might fail - instead they should block.  They can do this by
> waiting for a lock on the inode.
> 
> A new interface rmdir_lock() locks the inode, sets this flag, and waits
> for any children with DCACHE_LOCK set to complete their operation, and
> for any d_in_lookup() children to complete the lookup.  It should be
> called before attempted to delete the directory or set S_DEAD.  Matching
> rmdir_unlock() clears the flag and unlocks the inode.
> 
> dentry_lock() and d_alloc_parallel() are changed to block while this
> flag it set and to fail if the parent IS_DEADDIR(), though dentry_lock()
> doesn't block for d_in_lookup() dentries.

> diff --git a/fs/namei.c b/fs/namei.c
> index 4ad76df21677..c590f25d0d49 100644
> --- a/fs/namei.c
> +++ b/fs/namei.c
> @@ -1770,8 +1770,11 @@ static bool __dentry_lock(struct dentry *dentry,
>  			  struct dentry *base, const struct qstr *last,
>  			  unsigned int subclass, int state)
>  {
> +	struct dentry *parent;
> +	struct inode *dir;
>  	int err;
>  
> +retry:
>  	lock_acquire_exclusive(&dentry->dentry_map, subclass, 0, NULL, _THIS_IP_);
>  	spin_lock(&dentry->d_lock);
>  	err = wait_var_event_any_lock(&dentry->d_flags,
> @@ -1782,10 +1785,43 @@ static bool __dentry_lock(struct dentry *dentry,
>  		spin_unlock(&dentry->d_lock);
>  		return false;
>  	}
> -
> -	dentry->d_flags |= DCACHE_LOCK;
> +	parent = dentry->d_parent;

Why will it stay the parent?  Matter of fact, why will it stay positive?

> +	dir = igrab(parent->d_inode);

... and not oops right here?

> +	lock_map_release(&dentry->dentry_map);
>  	spin_unlock(&dentry->d_lock);
> -	return true;
> +
> +	if (state == TASK_KILLABLE) {
> +		err = down_write_killable(&dir->i_rwsem);
> +		if (err) {
> +			iput(dir);
> +			return false;
> +		}
> +	} else
> +		inode_lock(dir);
> +	/* S_DYING much be clear now */
> +	inode_unlock(dir);
> +	iput(dir);
> +	goto retry;

OK, I'm really confused now.  Is it allowed to call dentry_lock() while holding
->i_rwsem of the parent?

Where does your dentry lock nest wrt ->i_rwsem?  As a bonus (well, malus, I guess)
question, where does it nest wrt parent *and* child inodes' ->i_rwsem for rmdir
and rename?

Tangentially connected question: which locks are held for ->unlink() in your
scheme?  You do need *something* on the victim inode to protect ->i_nlink
modifications, and anything on dentries of victim or their parent directories
is not going to give that.