Re: [PATCH 08/11] fs: add support for non-blocking timestamp updates

[Jan Kara <jack@suse.cz>]

On Tue 06-01-26 08:50:02, Christoph Hellwig wrote:
> Currently file_update_time_flags unconditionally returns -EAGAIN if any
> timestamp needs to be updated and IOCB_NOWAIT is passed.  This makes
> non-blocking direct writes impossible on file systems with granular
> enough timestamps.
> 
> Pass IOCB_NOWAIT to ->update_time and return -EAGAIN if it could block.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Much nicer now! Thanks for the refactoring. Feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

								Honza

> ---
>  fs/btrfs/inode.c     |  2 ++
>  fs/gfs2/inode.c      |  3 +++
>  fs/inode.c           | 45 +++++++++++++++++++++++++++++++++-----------
>  fs/orangefs/inode.c  |  3 +++
>  fs/overlayfs/inode.c |  2 ++
>  fs/ubifs/file.c      |  3 +++
>  fs/xfs/xfs_iops.c    |  3 +++
>  7 files changed, 50 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 23fc38de9be5..241727459c0a 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6362,6 +6362,8 @@ static int btrfs_update_time(struct inode *inode, enum fs_update_time type,
>  
>  	if (btrfs_root_readonly(root))
>  		return -EROFS;
> +	if (flags & IOCB_NOWAIT)
> +		return -EAGAIN;
>  
>  	dirty = inode_update_time(inode, type, flags);
>  	if (dirty <= 0)
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 4ef39ff6889d..c02ebf0ca625 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -2250,6 +2250,9 @@ static int gfs2_update_time(struct inode *inode, enum fs_update_time type,
>  	struct gfs2_holder *gh;
>  	int error;
>  
> +	if (flags & IOCB_NOWAIT)
> +		return -EAGAIN;
> +
>  	gh = gfs2_glock_is_locked_by_me(gl);
>  	if (gh && gl->gl_state != LM_ST_EXCLUSIVE) {
>  		gfs2_glock_dq(gh);
> diff --git a/fs/inode.c b/fs/inode.c
> index c08682524a8d..01e4f6b9b46e 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2090,7 +2090,7 @@ static int inode_update_atime(struct inode *inode)
>  	return inode_time_dirty_flag(inode);
>  }
>  
> -static int inode_update_cmtime(struct inode *inode)
> +static int inode_update_cmtime(struct inode *inode, unsigned int flags)
>  {
>  	struct timespec64 now = inode_set_ctime_current(inode);
>  	struct timespec64 ctime = inode_get_ctime(inode);
> @@ -2101,12 +2101,27 @@ static int inode_update_cmtime(struct inode *inode)
>  	mtime_changed = !timespec64_equal(&now, &mtime);
>  	if (mtime_changed || !timespec64_equal(&now, &ctime))
>  		dirty = inode_time_dirty_flag(inode);
> -	if (mtime_changed)
> -		inode_set_mtime_to_ts(inode, now);
>  
> -	if (IS_I_VERSION(inode) && inode_maybe_inc_iversion(inode, !!dirty))
> -		dirty |= I_DIRTY_SYNC;
> +	/*
> +	 * Pure timestamp updates can be recorded in the inode without blocking
> +	 * by not dirtying the inode.  But when the file system requires
> +	 * i_version updates, the update of i_version can still block.
> +	 * Error out if we'd actually have to update i_version or don't support
> +	 * lazytime.
> +	 */
> +	if (IS_I_VERSION(inode)) {
> +		if (flags & IOCB_NOWAIT) {
> +			if (!(inode->i_sb->s_flags & SB_LAZYTIME) ||
> +			    inode_iversion_need_inc(inode))
> +				return -EAGAIN;
> +		} else {
> +			if (inode_maybe_inc_iversion(inode, !!dirty))
> +				dirty |= I_DIRTY_SYNC;
> +		}
> +	}
>  
> +	if (mtime_changed)
> +		inode_set_mtime_to_ts(inode, now);
>  	return dirty;
>  }
>  
> @@ -2131,7 +2146,7 @@ int inode_update_time(struct inode *inode, enum fs_update_time type,
>  	case FS_UPD_ATIME:
>  		return inode_update_atime(inode);
>  	case FS_UPD_CMTIME:
> -		return inode_update_cmtime(inode);
> +		return inode_update_cmtime(inode, flags);
>  	default:
>  		WARN_ON_ONCE(1);
>  		return -EIO;
> @@ -2152,6 +2167,16 @@ int generic_update_time(struct inode *inode, enum fs_update_time type,
>  {
>  	int dirty;
>  
> +	/*
> +	 * ->dirty_inode is what could make generic timestamp updates block.
> +	 * Don't support non-blocking timestamp updates here if it is set.
> +	 * File systems that implement ->dirty_inode but want to support
> +	 * non-blocking timestamp updates should call inode_update_time
> +	 * directly.
> +	 */
> +	if ((flags & IOCB_NOWAIT) && inode->i_sb->s_op->dirty_inode)
> +		return -EAGAIN;
> +
>  	dirty = inode_update_time(inode, type, flags);
>  	if (dirty <= 0)
>  		return dirty;
> @@ -2380,15 +2405,13 @@ static int file_update_time_flags(struct file *file, unsigned int flags)
>  	if (!need_update)
>  		return 0;
>  
> -	if (flags & IOCB_NOWAIT)
> -		return -EAGAIN;
> -
> +	flags &= IOCB_NOWAIT;
>  	if (mnt_get_write_access_file(file))
>  		return 0;
>  	if (inode->i_op->update_time)
> -		ret = inode->i_op->update_time(inode, FS_UPD_CMTIME, 0);
> +		ret = inode->i_op->update_time(inode, FS_UPD_CMTIME, flags);
>  	else
> -		ret = generic_update_time(inode, FS_UPD_CMTIME, 0);
> +		ret = generic_update_time(inode, FS_UPD_CMTIME, flags);
>  	mnt_put_write_access_file(file);
>  	return ret;
>  }
> diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> index eab16afb5b8a..f420f48fc069 100644
> --- a/fs/orangefs/inode.c
> +++ b/fs/orangefs/inode.c
> @@ -878,6 +878,9 @@ int orangefs_update_time(struct inode *inode, enum fs_update_time type,
>  	struct iattr iattr = { };
>  	int dirty;
>  
> +	if (flags & IOCB_NOWAIT)
> +		return -EAGAIN;
> +
>  	switch (type) {
>  	case FS_UPD_ATIME:
>  		iattr.ia_valid = ATTR_ATIME;
> diff --git a/fs/overlayfs/inode.c b/fs/overlayfs/inode.c
> index c0ce3519e4af..00c69707bda9 100644
> --- a/fs/overlayfs/inode.c
> +++ b/fs/overlayfs/inode.c
> @@ -566,6 +566,8 @@ int ovl_update_time(struct inode *inode, enum fs_update_time type,
>  		};
>  
>  		if (upperpath.dentry) {
> +			if (flags & IOCB_NOWAIT)
> +				return -EAGAIN;
>  			touch_atime(&upperpath);
>  			inode_set_atime_to_ts(inode,
>  					      inode_get_atime(d_inode(upperpath.dentry)));
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index 0cc44ad142de..3dc3ca1cd803 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1377,6 +1377,9 @@ int ubifs_update_time(struct inode *inode, enum fs_update_time type,
>  	if (!IS_ENABLED(CONFIG_UBIFS_ATIME_SUPPORT))
>  		return generic_update_time(inode, type, flags);
>  
> +	if (flags & IOCB_NOWAIT)
> +		return -EAGAIN;
> +
>  	err = ubifs_budget_space(c, &req);
>  	if (err)
>  		return err;
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index d9eae1af14a8..aef5b05c1b76 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -1195,6 +1195,9 @@ xfs_vn_update_time(
>  
>  	trace_xfs_update_time(ip);
>  
> +	if (flags & IOCB_NOWAIT)
> +		return -EAGAIN;
> +
>  	if (inode->i_sb->s_flags & SB_LAZYTIME) {
>  		if (type == FS_UPD_ATIME ||
>  		    !inode_maybe_inc_iversion(inode, false))
> -- 
> 2.47.3
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR