* two small setattr cleanups
@ 2020-12-10 5:48 Christoph Hellwig
2020-12-10 5:48 ` [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize Christoph Hellwig
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Christoph Hellwig @ 2020-12-10 5:48 UTC (permalink / raw)
To: linux-xfs
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.
^ permalink raw reply [flat|nested] 8+ messages in thread* [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize 2020-12-10 5:48 two small setattr cleanups Christoph Hellwig @ 2020-12-10 5:48 ` Christoph Hellwig 2020-12-10 7:42 ` Gao Xiang 2020-12-11 16:16 ` Brian Foster 2020-12-10 5:48 ` [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl Christoph Hellwig 2021-01-20 19:39 ` two small setattr cleanups Darrick J. Wong 2 siblings, 2 replies; 8+ messages in thread From: Christoph Hellwig @ 2020-12-10 5:48 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize 2020-12-10 5:48 ` [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize Christoph Hellwig @ 2020-12-10 7:42 ` Gao Xiang 2020-12-11 16:16 ` Brian Foster 1 sibling, 0 replies; 8+ messages in thread From: Gao Xiang @ 2020-12-10 7:42 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize 2020-12-10 5:48 ` [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize Christoph Hellwig 2020-12-10 7:42 ` Gao Xiang @ 2020-12-11 16:16 ` Brian Foster 1 sibling, 0 replies; 8+ messages in thread From: Brian Foster @ 2020-12-11 16:16 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl 2020-12-10 5:48 two small setattr cleanups Christoph Hellwig 2020-12-10 5:48 ` [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize Christoph Hellwig @ 2020-12-10 5:48 ` Christoph Hellwig 2020-12-10 7:48 ` Gao Xiang 2020-12-11 16:17 ` Brian Foster 2021-01-20 19:39 ` two small setattr cleanups Darrick J. Wong 2 siblings, 2 replies; 8+ messages in thread From: Christoph Hellwig @ 2020-12-10 5:48 UTC (permalink / raw) To: linux-xfs 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl 2020-12-10 5:48 ` [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl Christoph Hellwig @ 2020-12-10 7:48 ` Gao Xiang 2020-12-11 16:17 ` Brian Foster 1 sibling, 0 replies; 8+ messages in thread From: Gao Xiang @ 2020-12-10 7:48 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl 2020-12-10 5:48 ` [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl Christoph Hellwig 2020-12-10 7:48 ` Gao Xiang @ 2020-12-11 16:17 ` Brian Foster 1 sibling, 0 replies; 8+ messages in thread From: Brian Foster @ 2020-12-11 16:17 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: two small setattr cleanups 2020-12-10 5:48 two small setattr cleanups Christoph Hellwig 2020-12-10 5:48 ` [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize Christoph Hellwig 2020-12-10 5:48 ` [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl Christoph Hellwig @ 2021-01-20 19:39 ` Darrick J. Wong 2 siblings, 0 replies; 8+ messages in thread From: Darrick J. Wong @ 2021-01-20 19:39 UTC (permalink / raw) To: Christoph Hellwig; +Cc: linux-xfs 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-01-20 19:42 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-12-10 5:48 two small setattr cleanups Christoph Hellwig 2020-12-10 5:48 ` [PATCH 1/2] xfs: remove xfs_vn_setattr_nonsize Christoph Hellwig 2020-12-10 7:42 ` Gao Xiang 2020-12-11 16:16 ` Brian Foster 2020-12-10 5:48 ` [PATCH 2/2] xfs: open code updating i_mode in xfs_set_acl Christoph Hellwig 2020-12-10 7:48 ` Gao Xiang 2020-12-11 16:17 ` Brian Foster 2021-01-20 19:39 ` two small setattr cleanups Darrick J. Wong
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).