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@xxxxxx>
> ---
> 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
>
