Re: [PATCH 2/2] fs: make several inode lock operations killable

[Christian Brauner <brauner@kernel.org>]

On Tue, Apr 29, 2025 at 11:46:44AM +0200, Max Kellermann wrote:
> Allows killing processes that are waiting for the inode lock.
> 
> Signed-off-by: Max Kellermann <max.kellermann@ionos.com>
> ---

Oha, that's interesting.

>  fs/open.c       | 14 +++++++++++---
>  fs/read_write.c |  4 +++-
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/open.c b/fs/open.c
> index a9063cca9911..7828234a7caa 100644
> --- a/fs/open.c
> +++ b/fs/open.c
> @@ -60,7 +60,10 @@ int do_truncate(struct mnt_idmap *idmap, struct dentry *dentry,
>  	if (ret)
>  		newattrs.ia_valid |= ret | ATTR_FORCE;
>  
> -	inode_lock(dentry->d_inode);
> +	ret = inode_lock_killable(dentry->d_inode);
> +	if (ret)
> +		return ret;
> +
>  	/* Note any delegations or leases have already been broken: */
>  	ret = notify_change(idmap, dentry, &newattrs, NULL);
>  	inode_unlock(dentry->d_inode);
> @@ -635,7 +638,9 @@ int chmod_common(const struct path *path, umode_t mode)
>  	if (error)
>  		return error;
>  retry_deleg:
> -	inode_lock(inode);
> +	error = inode_lock_killable(inode);

That's probably fine.

> +	if (error)
> +		goto out_mnt_unlock;
>  	error = security_path_chmod(path, mode);
>  	if (error)
>  		goto out_unlock;
> @@ -650,6 +655,7 @@ int chmod_common(const struct path *path, umode_t mode)
>  		if (!error)
>  			goto retry_deleg;
>  	}
> +out_mnt_unlock:
>  	mnt_drop_write(path->mnt);
>  	return error;
>  }
> @@ -769,7 +775,9 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
>  		return -EINVAL;
>  	if ((group != (gid_t)-1) && !setattr_vfsgid(&newattrs, gid))
>  		return -EINVAL;
> -	inode_lock(inode);
> +	error = inode_lock_killable(inode);
> +	if (error)
> +		return error;

That too.

>  	if (!S_ISDIR(inode->i_mode))
>  		newattrs.ia_valid |= ATTR_KILL_SUID | ATTR_KILL_PRIV |
>  				     setattr_should_drop_sgid(idmap, inode);
> diff --git a/fs/read_write.c b/fs/read_write.c
> index bb0ed26a0b3a..0ef70e128c4a 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -332,7 +332,9 @@ loff_t default_llseek(struct file *file, loff_t offset, int whence)
>  	struct inode *inode = file_inode(file);
>  	loff_t retval;
>  
> -	inode_lock(inode);
> +	retval = inode_lock_killable(inode);

That change doesn't seem so obviously fine to me.
Either way I'd like to see this split in three patches and some
reasoning why it's safe and some justification why it's wanted...