Re: [PATCH 05/10] fs: allow error returns from inode_update_timestamps

On Wed 17-12-25 07:09:38, Christoph Hellwig wrote:
> Change flags to a by reference argument so that it can be updated so that
> the return value can be used for error returns.  This will be used to
> implement non-blocking timestamp updates.
> 
> Signed-off-by: Christoph Hellwig <[email protected]>
> Reviewed-by: Jeff Layton <[email protected]>

Looks good. Feel free to add:

Reviewed-by: Jan Kara <[email protected]>

								Honza

> ---
>  fs/btrfs/inode.c    |  8 +++++---
>  fs/inode.c          | 24 ++++++++++++++++--------
>  fs/nfs/inode.c      |  4 ++--
>  fs/orangefs/inode.c |  5 ++++-
>  fs/ubifs/file.c     |  2 +-
>  include/linux/fs.h  |  2 +-
>  6 files changed, 29 insertions(+), 16 deletions(-)
> 
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index 317db7d10a21..3ca8d294770e 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -6349,13 +6349,15 @@ static int btrfs_dirty_inode(struct btrfs_inode *inode)
>  static int btrfs_update_time(struct inode *inode, int flags)
>  {
>  	struct btrfs_root *root = BTRFS_I(inode)->root;
> -	bool dirty;
> +	int error;
>  
>  	if (btrfs_root_readonly(root))
>  		return -EROFS;
>  
> -	dirty = inode_update_timestamps(inode, flags);
> -	return dirty ? btrfs_dirty_inode(BTRFS_I(inode)) : 0;
> +	error = inode_update_timestamps(inode, &flags);
> +	if (error || !flags)
> +		return error;
> +	return btrfs_dirty_inode(BTRFS_I(inode));
>  }
>  
>  /*
> diff --git a/fs/inode.c b/fs/inode.c
> index 17ecb7bb5067..2c0d69f7fd01 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -2095,14 +2095,18 @@ static bool relatime_need_update(struct vfsmount *mnt, struct inode *inode,
>   * attempt to update all three of them. S_ATIME updates can be handled
>   * independently of the rest.
>   *
> - * Returns a set of S_* flags indicating which values changed.
> + * Updates @flags to contain the S_* flags which actually need changing.  This
> + * can drop flags from the input when they don't need an update, or can add
> + * S_VERSION when the version needs to be bumped.
> + *
> + * Returns 0 or a negative errno.
>   */
> -int inode_update_timestamps(struct inode *inode, int flags)
> +int inode_update_timestamps(struct inode *inode, int *flags)
>  {
>  	int updated = 0;
>  	struct timespec64 now;
>  
> -	if (flags & (S_MTIME|S_CTIME|S_VERSION)) {
> +	if (*flags & (S_MTIME | S_CTIME | S_VERSION)) {
>  		struct timespec64 ctime = inode_get_ctime(inode);
>  		struct timespec64 mtime = inode_get_mtime(inode);
>  
> @@ -2119,7 +2123,7 @@ int inode_update_timestamps(struct inode *inode, int flags)
>  		now = current_time(inode);
>  	}
>  
> -	if (flags & S_ATIME) {
> +	if (*flags & S_ATIME) {
>  		struct timespec64 atime = inode_get_atime(inode);
>  
>  		if (!timespec64_equal(&now, &atime)) {
> @@ -2127,7 +2131,9 @@ int inode_update_timestamps(struct inode *inode, int flags)
>  			updated |= S_ATIME;
>  		}
>  	}
> -	return updated;
> +
> +	*flags = updated;
> +	return 0;
>  }
>  EXPORT_SYMBOL(inode_update_timestamps);
>  
> @@ -2145,10 +2151,12 @@ EXPORT_SYMBOL(inode_update_timestamps);
>   */
>  int generic_update_time(struct inode *inode, int flags)
>  {
> -	flags = inode_update_timestamps(inode, flags);
> -	if (flags)
> +	int error;
> +
> +	error = inode_update_timestamps(inode, &flags);
> +	if (!error && flags)
>  		mark_inode_dirty_time(inode, flags);
> -	return 0;
> +	return error;
>  }
>  EXPORT_SYMBOL(generic_update_time);
>  
> diff --git a/fs/nfs/inode.c b/fs/nfs/inode.c
> index 84049f3cd340..221816524c66 100644
> --- a/fs/nfs/inode.c
> +++ b/fs/nfs/inode.c
> @@ -671,8 +671,8 @@ static void nfs_set_timestamps_to_ts(struct inode *inode, struct iattr *attr)
>  
>  static void nfs_update_timestamps(struct inode *inode, unsigned int ia_valid)
>  {
> -	enum file_time_flags time_flags = 0;
>  	unsigned int cache_flags = 0;
> +	int time_flags = 0;
>  
>  	if (ia_valid & ATTR_MTIME) {
>  		time_flags |= S_MTIME | S_CTIME;
> @@ -682,7 +682,7 @@ static void nfs_update_timestamps(struct inode *inode, unsigned int ia_valid)
>  		time_flags |= S_ATIME;
>  		cache_flags |= NFS_INO_INVALID_ATIME;
>  	}
> -	inode_update_timestamps(inode, time_flags);
> +	inode_update_timestamps(inode, &time_flags);
>  	NFS_I(inode)->cache_validity &= ~cache_flags;
>  }
>  
> diff --git a/fs/orangefs/inode.c b/fs/orangefs/inode.c
> index d7275990ffa4..3b58f31bd54f 100644
> --- a/fs/orangefs/inode.c
> +++ b/fs/orangefs/inode.c
> @@ -875,11 +875,14 @@ int orangefs_permission(struct mnt_idmap *idmap,
>  int orangefs_update_time(struct inode *inode, int flags)
>  {
>  	struct iattr iattr;
> +	int error;
>  
>  	gossip_debug(GOSSIP_INODE_DEBUG, "orangefs_update_time: %pU\n",
>  	    get_khandle_from_ino(inode));
>  
> -	flags = inode_update_timestamps(inode, flags);
> +	error = inode_update_timestamps(inode, &flags);
> +	if (error || !flags)
> +		return error;
>  
>  	memset(&iattr, 0, sizeof iattr);
>          if (flags & S_ATIME)
> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
> index ec1bb9f43acc..71540644a931 100644
> --- a/fs/ubifs/file.c
> +++ b/fs/ubifs/file.c
> @@ -1387,7 +1387,7 @@ int ubifs_update_time(struct inode *inode, int flags)
>  		return err;
>  
>  	mutex_lock(&ui->ui_mutex);
> -	inode_update_timestamps(inode, flags);
> +	inode_update_timestamps(inode, &flags);
>  	release = ui->dirty;
>  	__mark_inode_dirty(inode, I_DIRTY_SYNC);
>  	mutex_unlock(&ui->ui_mutex);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 66d3d18cf4e3..75d5f38b08c9 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -2399,7 +2399,7 @@ static inline void super_set_sysfs_name_generic(struct super_block *sb, const ch
>  extern void ihold(struct inode * inode);
>  extern void iput(struct inode *);
>  void iput_not_last(struct inode *);
> -int inode_update_timestamps(struct inode *inode, int flags);
> +int inode_update_timestamps(struct inode *inode, int *flags);
>  int generic_update_time(struct inode *inode, int flags);
>  
>  /* /sys/fs */
> -- 
> 2.47.3
> 
-- 
Jan Kara <[email protected]>
SUSE Labs, CR