two small setattr cleanups

two small setattr cleanups

[Christoph Hellwig]

Hi all,

this series contains two cleanups to the setattr patch.  I did
those in preparation for supporting idmapped mounts in XFS, but
I think they are useful on their own.

[PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize

[Christoph Hellwig]

Merge xfs_vn_setattr_nonsize into the only caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_iops.c | 26 +++++++-------------------
 fs/xfs/xfs_iops.h |  1 -
 2 files changed, 7 insertions(+), 20 deletions(-)

diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 1414ab79eacfc2..54c7c94f82951b 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -826,22 +826,6 @@ xfs_setattr_nonsize(
 	return error;
 }
 
-int
-xfs_vn_setattr_nonsize(
-	struct dentry		*dentry,
-	struct iattr		*iattr)
-{
-	struct xfs_inode	*ip = XFS_I(d_inode(dentry));
-	int error;
-
-	trace_xfs_setattr(ip);
-
-	error = xfs_vn_change_ok(dentry, iattr);
-	if (error)
-		return error;
-	return xfs_setattr_nonsize(ip, iattr, 0);
-}
-
 /*
  * Truncate file.  Must have write permission and not be a directory.
  *
@@ -1069,11 +1053,11 @@ xfs_vn_setattr(
 	struct dentry		*dentry,
 	struct iattr		*iattr)
 {
+	struct inode		*inode = d_inode(dentry);
+	struct xfs_inode	*ip = XFS_I(inode);
 	int			error;
 
 	if (iattr->ia_valid & ATTR_SIZE) {
-		struct inode		*inode = d_inode(dentry);
-		struct xfs_inode	*ip = XFS_I(inode);
 		uint			iolock;
 
 		xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
@@ -1088,7 +1072,11 @@ xfs_vn_setattr(
 		error = xfs_vn_setattr_size(dentry, iattr);
 		xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
 	} else {
-		error = xfs_vn_setattr_nonsize(dentry, iattr);
+		trace_xfs_setattr(ip);
+
+		error = xfs_vn_change_ok(dentry, iattr);
+		if (!error)
+			error = xfs_setattr_nonsize(ip, iattr, 0);
 	}
 
 	return error;
diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
index 4d24ff309f593f..a91e2d1b47b45d 100644
--- a/fs/xfs/xfs_iops.h
+++ b/fs/xfs/xfs_iops.h
@@ -21,7 +21,6 @@ extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
 extern void xfs_setattr_time(struct xfs_inode *ip, struct iattr *iattr);
 extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
 			       int flags);
-extern int xfs_vn_setattr_nonsize(struct dentry *dentry, struct iattr *vap);
 extern int xfs_vn_setattr_size(struct dentry *dentry, struct iattr *vap);
 
 #endif /* __XFS_IOPS_H__ */
-- 
2.29.2

Re: [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize

[Gao Xiang]

On Thu, Dec 10, 2020 at 06:48:20AM +0100, Christoph Hellwig wrote:
> Merge xfs_vn_setattr_nonsize into the only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me (if it's needed),
Reviewed-by: Gao Xiang <hsiangkao@redhat.com>

Thanks,
Gao Xiang

Re: [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize

[Brian Foster]

On Thu, Dec 10, 2020 at 06:48:20AM +0100, Christoph Hellwig wrote:
> Merge xfs_vn_setattr_nonsize into the only caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---

Reviewed-by: Brian Foster <bfoster@redhat.com>

>  fs/xfs/xfs_iops.c | 26 +++++++-------------------
>  fs/xfs/xfs_iops.h |  1 -
>  2 files changed, 7 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 1414ab79eacfc2..54c7c94f82951b 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -826,22 +826,6 @@ xfs_setattr_nonsize(
>  	return error;
>  }
>  
> -int
> -xfs_vn_setattr_nonsize(
> -	struct dentry		*dentry,
> -	struct iattr		*iattr)
> -{
> -	struct xfs_inode	*ip = XFS_I(d_inode(dentry));
> -	int error;
> -
> -	trace_xfs_setattr(ip);
> -
> -	error = xfs_vn_change_ok(dentry, iattr);
> -	if (error)
> -		return error;
> -	return xfs_setattr_nonsize(ip, iattr, 0);
> -}
> -
>  /*
>   * Truncate file.  Must have write permission and not be a directory.
>   *
> @@ -1069,11 +1053,11 @@ xfs_vn_setattr(
>  	struct dentry		*dentry,
>  	struct iattr		*iattr)
>  {
> +	struct inode		*inode = d_inode(dentry);
> +	struct xfs_inode	*ip = XFS_I(inode);
>  	int			error;
>  
>  	if (iattr->ia_valid & ATTR_SIZE) {
> -		struct inode		*inode = d_inode(dentry);
> -		struct xfs_inode	*ip = XFS_I(inode);
>  		uint			iolock;
>  
>  		xfs_ilock(ip, XFS_MMAPLOCK_EXCL);
> @@ -1088,7 +1072,11 @@ xfs_vn_setattr(
>  		error = xfs_vn_setattr_size(dentry, iattr);
>  		xfs_iunlock(ip, XFS_MMAPLOCK_EXCL);
>  	} else {
> -		error = xfs_vn_setattr_nonsize(dentry, iattr);
> +		trace_xfs_setattr(ip);
> +
> +		error = xfs_vn_change_ok(dentry, iattr);
> +		if (!error)
> +			error = xfs_setattr_nonsize(ip, iattr, 0);
>  	}
>  
>  	return error;
> diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
> index 4d24ff309f593f..a91e2d1b47b45d 100644
> --- a/fs/xfs/xfs_iops.h
> +++ b/fs/xfs/xfs_iops.h
> @@ -21,7 +21,6 @@ extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
>  extern void xfs_setattr_time(struct xfs_inode *ip, struct iattr *iattr);
>  extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
>  			       int flags);
> -extern int xfs_vn_setattr_nonsize(struct dentry *dentry, struct iattr *vap);
>  extern int xfs_vn_setattr_size(struct dentry *dentry, struct iattr *vap);
>  
>  #endif /* __XFS_IOPS_H__ */
> -- 
> 2.29.2
> 

[PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl

[Christoph Hellwig]

Rather than going through the big and hairy xfs_setattr_nonsize function,
just open code a transactional i_mode and i_ctime update.  This allows
to mark xfs_setattr_nonsize and remove the flags argument to it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_acl.c  | 40 ++++++++++++++++++++++------------------
 fs/xfs/xfs_iops.c | 11 +++++------
 fs/xfs/xfs_iops.h |  7 -------
 3 files changed, 27 insertions(+), 31 deletions(-)

diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
index c544951a0c07f3..779cb73b3d006f 100644
--- a/fs/xfs/xfs_acl.c
+++ b/fs/xfs/xfs_acl.c
@@ -16,6 +16,7 @@
 #include "xfs_acl.h"
 #include "xfs_da_format.h"
 #include "xfs_da_btree.h"
+#include "xfs_trans.h"
 
 #include <linux/posix_acl_xattr.h>
 
@@ -212,21 +213,28 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 }
 
 static int
-xfs_set_mode(struct inode *inode, umode_t mode)
+xfs_acl_set_mode(
+	struct inode		*inode,
+	umode_t			mode)
 {
-	int error = 0;
-
-	if (mode != inode->i_mode) {
-		struct iattr iattr;
+	struct xfs_inode	*ip = XFS_I(inode);
+	struct xfs_mount	*mp = ip->i_mount;
+	struct xfs_trans	*tp;
+	int			error;
 
-		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
-		iattr.ia_mode = mode;
-		iattr.ia_ctime = current_time(inode);
+	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
+	if (error)
+		return error;
 
-		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
-	}
+	xfs_ilock(ip, XFS_ILOCK_EXCL);
+	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+	inode->i_mode = mode;
+	inode->i_ctime = current_time(inode);
+	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
 
-	return error;
+	if (mp->m_flags & XFS_MOUNT_WSYNC)
+		xfs_trans_set_sync(tp);
+	return xfs_trans_commit(tp);
 }
 
 int
@@ -251,18 +259,14 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
 	}
 
  set_acl:
-	error =  __xfs_set_acl(inode, acl, type);
-	if (error)
-		return error;
-
 	/*
 	 * We set the mode after successfully updating the ACL xattr because the
 	 * xattr update can fail at ENOSPC and we don't want to change the mode
 	 * if the ACL update hasn't been applied.
 	 */
-	if (set_mode)
-		error = xfs_set_mode(inode, mode);
-
+	error =  __xfs_set_acl(inode, acl, type);
+	if (!error && set_mode && mode != inode->i_mode)
+		error = xfs_acl_set_mode(inode, mode);
 	return error;
 }
 
diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
index 54c7c94f82951b..88d6dbeb81e9ca 100644
--- a/fs/xfs/xfs_iops.c
+++ b/fs/xfs/xfs_iops.c
@@ -648,11 +648,10 @@ xfs_vn_change_ok(
  * Caution: The caller of this function is responsible for calling
  * setattr_prepare() or otherwise verifying the change is fine.
  */
-int
+static int
 xfs_setattr_nonsize(
 	struct xfs_inode	*ip,
-	struct iattr		*iattr,
-	int			flags)
+	struct iattr		*iattr)
 {
 	xfs_mount_t		*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
@@ -809,7 +808,7 @@ xfs_setattr_nonsize(
 	 *	     to attr_set.  No previous user of the generic
 	 * 	     Posix ACL code seems to care about this issue either.
 	 */
-	if ((mask & ATTR_MODE) && !(flags & XFS_ATTR_NOACL)) {
+	if (mask & ATTR_MODE) {
 		error = posix_acl_chmod(inode, inode->i_mode);
 		if (error)
 			return error;
@@ -865,7 +864,7 @@ xfs_setattr_size(
 		 * Use the regular setattr path to update the timestamps.
 		 */
 		iattr->ia_valid &= ~ATTR_SIZE;
-		return xfs_setattr_nonsize(ip, iattr, 0);
+		return xfs_setattr_nonsize(ip, iattr);
 	}
 
 	/*
@@ -1076,7 +1075,7 @@ xfs_vn_setattr(
 
 		error = xfs_vn_change_ok(dentry, iattr);
 		if (!error)
-			error = xfs_setattr_nonsize(ip, iattr, 0);
+			error = xfs_setattr_nonsize(ip, iattr);
 	}
 
 	return error;
diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
index a91e2d1b47b45d..99ca745c1071bf 100644
--- a/fs/xfs/xfs_iops.h
+++ b/fs/xfs/xfs_iops.h
@@ -13,14 +13,7 @@ extern const struct file_operations xfs_dir_file_operations;
 
 extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
 
-/*
- * Internal setattr interfaces.
- */
-#define XFS_ATTR_NOACL		0x01	/* Don't call posix_acl_chmod */
-
 extern void xfs_setattr_time(struct xfs_inode *ip, struct iattr *iattr);
-extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
-			       int flags);
 extern int xfs_vn_setattr_size(struct dentry *dentry, struct iattr *vap);
 
 #endif /* __XFS_IOPS_H__ */
-- 
2.29.2

Re: [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl

[Gao Xiang]

On Thu, Dec 10, 2020 at 06:48:21AM +0100, Christoph Hellwig wrote:
> Rather than going through the big and hairy xfs_setattr_nonsize function,
> just open code a transactional i_mode and i_ctime update.  This allows
> to mark xfs_setattr_nonsize and remove the flags argument to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good to me (if it's needed),
Reviewed-by: Gao Xiang <hsiangkao@redhat.com>

Thanks,
Gao Xiang

Re: [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl

[Brian Foster]

On Thu, Dec 10, 2020 at 06:48:21AM +0100, Christoph Hellwig wrote:
> Rather than going through the big and hairy xfs_setattr_nonsize function,
> just open code a transactional i_mode and i_ctime update.  This allows
> to mark xfs_setattr_nonsize and remove the flags argument to it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_acl.c  | 40 ++++++++++++++++++++++------------------
>  fs/xfs/xfs_iops.c | 11 +++++------
>  fs/xfs/xfs_iops.h |  7 -------
>  3 files changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/xfs/xfs_acl.c b/fs/xfs/xfs_acl.c
> index c544951a0c07f3..779cb73b3d006f 100644
> --- a/fs/xfs/xfs_acl.c
> +++ b/fs/xfs/xfs_acl.c
...
> @@ -212,21 +213,28 @@ __xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  }
>  
>  static int
> -xfs_set_mode(struct inode *inode, umode_t mode)
> +xfs_acl_set_mode(
> +	struct inode		*inode,
> +	umode_t			mode)
>  {
> -	int error = 0;
> -
> -	if (mode != inode->i_mode) {
> -		struct iattr iattr;
> +	struct xfs_inode	*ip = XFS_I(inode);
> +	struct xfs_mount	*mp = ip->i_mount;
> +	struct xfs_trans	*tp;
> +	int			error;
>  
> -		iattr.ia_valid = ATTR_MODE | ATTR_CTIME;
> -		iattr.ia_mode = mode;
> -		iattr.ia_ctime = current_time(inode);
> +	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> +	if (error)
> +		return error;
>  
> -		error = xfs_setattr_nonsize(XFS_I(inode), &iattr, XFS_ATTR_NOACL);
> -	}
> +	xfs_ilock(ip, XFS_ILOCK_EXCL);
> +	xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> +	inode->i_mode = mode;

Do we need to care about carrying the S_IFMT protection logic in
xfs_setattr_mode()? It looks like perhaps not from a quick scan through
the acl helper, but want to confirm this change was intentional. If so,
explanation in the commit log would be nice for historical clarity.
Otherwise looks reasonable..

Brian

> +	inode->i_ctime = current_time(inode);
> +	xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
>  
> -	return error;
> +	if (mp->m_flags & XFS_MOUNT_WSYNC)
> +		xfs_trans_set_sync(tp);
> +	return xfs_trans_commit(tp);
>  }
>  
>  int
> @@ -251,18 +259,14 @@ xfs_set_acl(struct inode *inode, struct posix_acl *acl, int type)
>  	}
>  
>   set_acl:
> -	error =  __xfs_set_acl(inode, acl, type);
> -	if (error)
> -		return error;
> -
>  	/*
>  	 * We set the mode after successfully updating the ACL xattr because the
>  	 * xattr update can fail at ENOSPC and we don't want to change the mode
>  	 * if the ACL update hasn't been applied.
>  	 */
> -	if (set_mode)
> -		error = xfs_set_mode(inode, mode);
> -
> +	error =  __xfs_set_acl(inode, acl, type);
> +	if (!error && set_mode && mode != inode->i_mode)
> +		error = xfs_acl_set_mode(inode, mode);
>  	return error;
>  }
>  
> diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> index 54c7c94f82951b..88d6dbeb81e9ca 100644
> --- a/fs/xfs/xfs_iops.c
> +++ b/fs/xfs/xfs_iops.c
> @@ -648,11 +648,10 @@ xfs_vn_change_ok(
>   * Caution: The caller of this function is responsible for calling
>   * setattr_prepare() or otherwise verifying the change is fine.
>   */
> -int
> +static int
>  xfs_setattr_nonsize(
>  	struct xfs_inode	*ip,
> -	struct iattr		*iattr,
> -	int			flags)
> +	struct iattr		*iattr)
>  {
>  	xfs_mount_t		*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
> @@ -809,7 +808,7 @@ xfs_setattr_nonsize(
>  	 *	     to attr_set.  No previous user of the generic
>  	 * 	     Posix ACL code seems to care about this issue either.
>  	 */
> -	if ((mask & ATTR_MODE) && !(flags & XFS_ATTR_NOACL)) {
> +	if (mask & ATTR_MODE) {
>  		error = posix_acl_chmod(inode, inode->i_mode);
>  		if (error)
>  			return error;
> @@ -865,7 +864,7 @@ xfs_setattr_size(
>  		 * Use the regular setattr path to update the timestamps.
>  		 */
>  		iattr->ia_valid &= ~ATTR_SIZE;
> -		return xfs_setattr_nonsize(ip, iattr, 0);
> +		return xfs_setattr_nonsize(ip, iattr);
>  	}
>  
>  	/*
> @@ -1076,7 +1075,7 @@ xfs_vn_setattr(
>  
>  		error = xfs_vn_change_ok(dentry, iattr);
>  		if (!error)
> -			error = xfs_setattr_nonsize(ip, iattr, 0);
> +			error = xfs_setattr_nonsize(ip, iattr);
>  	}
>  
>  	return error;
> diff --git a/fs/xfs/xfs_iops.h b/fs/xfs/xfs_iops.h
> index a91e2d1b47b45d..99ca745c1071bf 100644
> --- a/fs/xfs/xfs_iops.h
> +++ b/fs/xfs/xfs_iops.h
> @@ -13,14 +13,7 @@ extern const struct file_operations xfs_dir_file_operations;
>  
>  extern ssize_t xfs_vn_listxattr(struct dentry *, char *data, size_t size);
>  
> -/*
> - * Internal setattr interfaces.
> - */
> -#define XFS_ATTR_NOACL		0x01	/* Don't call posix_acl_chmod */
> -
>  extern void xfs_setattr_time(struct xfs_inode *ip, struct iattr *iattr);
> -extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
> -			       int flags);
>  extern int xfs_vn_setattr_size(struct dentry *dentry, struct iattr *vap);
>  
>  #endif /* __XFS_IOPS_H__ */
> -- 
> 2.29.2
> 

Re: two small setattr cleanups

[Darrick J. Wong]

On Thu, Dec 10, 2020 at 06:48:19AM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> this series contains two cleanups to the setattr patch.  I did
> those in preparation for supporting idmapped mounts in XFS, but
> I think they are useful on their own.

Apparently I never sent an official RVB on this before merging it...

Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D