From: Timothy Shimmin <tes@sgi.com>
To: Christoph Hellwig <hch@lst.de>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 3/3] kill vn_revalidate
Date: Tue, 15 Jul 2008 16:36:29 +1000 [thread overview]
Message-ID: <487C456D.3010509@sgi.com> (raw)
In-Reply-To: <20080705172110.GB7177@lst.de>
Hi,
Looks reasonable.
Looking at changes...
* remove calls to vn_revalidate from:
-> xfs_ioctl_setattr (but call xfs_diflags_to_linux(ip) for FSX_XFLAGS instead
which is the relevant bit of vn_revalidate)
-> xfs_dm_set_fileattr (after call to xfs_setattr)
-> xfs_vn_setattr (after call to xfs_setattr)
-> xfs_xattr_system_set
(after call to xfs_acl_vset -> xfs_acl_setmode -> xfs_setattr)
So we now do inode field sets in xfs_setattr instead of waiting until
after calling xfs_setattr (either directly or indirectly) to get vn_revalidate
to set the inode fields. Okay.
* in xfs_setattr we now explicitly set for linux inode:
i_mode, i_uid, i_gid, i_atime, i_mtime, i_ctime
It looks like we didn't set i_atime in vn_revalidate previously.
This change looks weird:
> */
> - iattr.ia_mask = XFS_AT_MODE;
> + iattr.ia_valid = ATTR_MODE;
> iattr.ia_mode = xfs_vtoi(vp)->i_d.di_mode;
I didn't think there was an ia_mask field - was this really
from another patch? Confused.
--Tim
Christoph Hellwig wrote:
> On Fri, Jun 27, 2008 at 05:46:02PM +0200, Christoph Hellwig wrote:
>> These days most of the attributes in struct inode are properly kept in
>> sync by XFS. This patch removes the need for vn_revalidate completely
>> by:
>>
>> - keeping inode.i_flags uptodate after any flags are updated in
>> xfs_ioctl_setattr
>> - keeping i_mode, i_uid and i_gid uptodate in xfs_setattr
>
> Version that doesn't require the generic ACL patch below:
>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Index: linux-2.6-xfs/fs/xfs/dmapi/xfs_dm.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/dmapi/xfs_dm.c 2008-07-05 10:04:56.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/dmapi/xfs_dm.c 2008-07-05 12:02:24.000000000 +0200
> @@ -2665,7 +2665,6 @@ xfs_dm_set_fileattr(
> {
> dm_fileattr_t stat;
> struct iattr iattr;
> - int error;
>
> /* Returns negative errors to DMAPI */
>
> @@ -2720,10 +2719,7 @@ xfs_dm_set_fileattr(
> iattr.ia_size = stat.fa_size;
> }
>
> - error = xfs_setattr(XFS_I(inode), &iattr, XFS_ATTR_DMI, NULL);
> - if (!error)
> - vn_revalidate(vn_from_inode(inode)); /* update Linux inode flags */
> - return(-error); /* Return negative error to DMAPI */
> + return -xfs_setattr(XFS_I(inode), &iattr, XFS_ATTR_DMI, NULL);
> }
>
>
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_ioctl.c 2008-07-05 10:04:56.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ioctl.c 2008-07-05 12:02:24.000000000 +0200
> @@ -920,6 +920,30 @@ xfs_set_diflags(
> ip->i_d.di_flags = di_flags;
> }
>
> +STATIC void
> +xfs_diflags_to_linux(
> + struct xfs_inode *ip)
> +{
> + struct inode *inode = XFS_ITOV(ip);
> + unsigned int xflags = xfs_ip2xflags(ip);
> +
> + if (xflags & XFS_XFLAG_IMMUTABLE)
> + inode->i_flags |= S_IMMUTABLE;
> + else
> + inode->i_flags &= ~S_IMMUTABLE;
> + if (xflags & XFS_XFLAG_APPEND)
> + inode->i_flags |= S_APPEND;
> + else
> + inode->i_flags &= ~S_APPEND;
> + if (xflags & XFS_XFLAG_SYNC)
> + inode->i_flags |= S_SYNC;
> + else
> + inode->i_flags &= ~S_SYNC;
> + if (xflags & XFS_XFLAG_NOATIME)
> + inode->i_flags |= S_NOATIME;
> + else
> + inode->i_flags &= ~S_NOATIME;
> +}
>
> #define FSX_PROJID 1
> #define FSX_EXTSIZE 2
> @@ -1118,8 +1142,10 @@ xfs_ioctl_setattr(
>
> if (mask & FSX_EXTSIZE)
> ip->i_d.di_extsize = fa->fsx_extsize >> mp->m_sb.sb_blocklog;
> - if (mask & FSX_XFLAGS)
> + if (mask & FSX_XFLAGS) {
> xfs_set_diflags(ip, fa->fsx_xflags);
> + xfs_diflags_to_linux(ip);
> + }
>
> xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> xfs_ichgtime(ip, XFS_ICHGTIME_CHG);
> @@ -1157,7 +1183,6 @@ xfs_ioctl_setattr(
> (mask & FSX_NONBLOCK) ? DM_FLAGS_NDELAY : 0);
> }
>
> - vn_revalidate(XFS_ITOV(ip)); /* update flags */
> return 0;
>
> error_return:
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_iops.c 2008-07-05 10:04:56.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_iops.c 2008-07-05 12:02:24.000000000 +0200
> @@ -658,21 +658,7 @@ xfs_vn_setattr(
> struct dentry *dentry,
> struct iattr *iattr)
> {
> - struct inode *inode = dentry->d_inode;
> - int error;
> -
> - if (iattr->ia_valid & ATTR_ATIME)
> - inode->i_atime = iattr->ia_atime;
> -
> - if (iattr->ia_valid & ATTR_MODE) {
> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> - inode->i_mode &= ~S_ISGID;
> - }
> -
> - error = xfs_setattr(XFS_I(inode), iattr, 0, NULL);
> - if (likely(!error))
> - vn_revalidate(vn_from_inode(inode));
> - return -error;
> + return -xfs_setattr(XFS_I(dentry->d_inode), iattr, 0, NULL);
> }
>
> /*
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ksyms.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_ksyms.c 2008-07-05 10:04:17.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_ksyms.c 2008-07-05 12:02:24.000000000 +0200
> @@ -177,7 +177,6 @@ EXPORT_SYMBOL(uuid_hash64);
> EXPORT_SYMBOL(uuid_is_nil);
> EXPORT_SYMBOL(uuid_table_remove);
> EXPORT_SYMBOL(vn_hold);
> -EXPORT_SYMBOL(vn_revalidate);
>
> #if defined(CONFIG_XFS_POSIX_ACL)
> EXPORT_SYMBOL(xfs_acl_vtoacl);
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_vnode.c 2008-07-05 10:02:09.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.c 2008-07-05 10:04:57.000000000 +0200
> @@ -82,56 +82,6 @@ vn_ioerror(
> xfs_do_force_shutdown(ip->i_mount, SHUTDOWN_DEVICE_REQ, f, l);
> }
>
> -/*
> - * Revalidate the Linux inode from the XFS inode.
> - * Note: i_size _not_ updated; we must hold the inode
> - * semaphore when doing that - callers responsibility.
> - */
> -int
> -vn_revalidate(
> - bhv_vnode_t *vp)
> -{
> - struct inode *inode = vn_to_inode(vp);
> - struct xfs_inode *ip = XFS_I(inode);
> - struct xfs_mount *mp = ip->i_mount;
> - unsigned long xflags;
> -
> - xfs_itrace_entry(ip);
> -
> - if (XFS_FORCED_SHUTDOWN(mp))
> - return -EIO;
> -
> - xfs_ilock(ip, XFS_ILOCK_SHARED);
> - inode->i_mode = ip->i_d.di_mode;
> - inode->i_uid = ip->i_d.di_uid;
> - inode->i_gid = ip->i_d.di_gid;
> - inode->i_mtime.tv_sec = ip->i_d.di_mtime.t_sec;
> - inode->i_mtime.tv_nsec = ip->i_d.di_mtime.t_nsec;
> - inode->i_ctime.tv_sec = ip->i_d.di_ctime.t_sec;
> - inode->i_ctime.tv_nsec = ip->i_d.di_ctime.t_nsec;
> -
> - xflags = xfs_ip2xflags(ip);
> - if (xflags & XFS_XFLAG_IMMUTABLE)
> - inode->i_flags |= S_IMMUTABLE;
> - else
> - inode->i_flags &= ~S_IMMUTABLE;
> - if (xflags & XFS_XFLAG_APPEND)
> - inode->i_flags |= S_APPEND;
> - else
> - inode->i_flags &= ~S_APPEND;
> - if (xflags & XFS_XFLAG_SYNC)
> - inode->i_flags |= S_SYNC;
> - else
> - inode->i_flags &= ~S_SYNC;
> - if (xflags & XFS_XFLAG_NOATIME)
> - inode->i_flags |= S_NOATIME;
> - else
> - inode->i_flags &= ~S_NOATIME;
> - xfs_iunlock(ip, XFS_ILOCK_SHARED);
> -
> - xfs_iflags_clear(ip, XFS_IMODIFIED);
> - return 0;
> -}
>
> /*
> * Add a reference to a referenced vnode.
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.h
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_vnode.h 2008-07-05 10:04:56.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_vnode.h 2008-07-05 10:04:57.000000000 +0200
> @@ -67,7 +67,6 @@ static inline struct inode *vn_to_inode(
>
>
> extern void vn_init(void);
> -extern int vn_revalidate(bhv_vnode_t *);
>
> /*
> * Yeah, these don't take vnode anymore at all, all this should be
> Index: linux-2.6-xfs/fs/xfs/xfs_vnodeops.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_vnodeops.c 2008-07-05 10:04:56.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_vnodeops.c 2008-07-05 10:04:57.000000000 +0200
> @@ -83,6 +83,7 @@ xfs_setattr(
> cred_t *credp)
> {
> xfs_mount_t *mp = ip->i_mount;
> + struct inode *inode = XFS_ITOV(ip);
> int mask = iattr->ia_valid;
> xfs_trans_t *tp;
> int code;
> @@ -446,6 +447,9 @@ xfs_setattr(
> ip->i_d.di_mode &= S_IFMT;
> ip->i_d.di_mode |= iattr->ia_mode & ~S_IFMT;
>
> + inode->i_mode &= S_IFMT;
> + inode->i_mode |= iattr->ia_mode & ~S_IFMT;
> +
> xfs_trans_log_inode (tp, ip, XFS_ILOG_CORE);
> timeflags |= XFS_ICHGTIME_CHG;
> }
> @@ -481,6 +485,7 @@ xfs_setattr(
> &ip->i_udquot, udqp);
> }
> ip->i_d.di_uid = uid;
> + inode->i_uid = uid;
> }
> if (igid != gid) {
> if (XFS_IS_GQUOTA_ON(mp)) {
> @@ -491,6 +496,7 @@ xfs_setattr(
> &ip->i_gdquot, gdqp);
> }
> ip->i_d.di_gid = gid;
> + inode->i_gid = gid;
> }
>
> xfs_trans_log_inode (tp, ip, XFS_ILOG_CORE);
> @@ -503,12 +509,14 @@ xfs_setattr(
> */
> if (mask & (ATTR_ATIME|ATTR_MTIME)) {
> if (mask & ATTR_ATIME) {
> + inode->i_atime = iattr->ia_atime;
> ip->i_d.di_atime.t_sec = iattr->ia_atime.tv_sec;
> ip->i_d.di_atime.t_nsec = iattr->ia_atime.tv_nsec;
> ip->i_update_core = 1;
> timeflags &= ~XFS_ICHGTIME_ACC;
> }
> if (mask & ATTR_MTIME) {
> + inode->i_mtime = iattr->ia_mtime;
> ip->i_d.di_mtime.t_sec = iattr->ia_mtime.tv_sec;
> ip->i_d.di_mtime.t_nsec = iattr->ia_mtime.tv_nsec;
> timeflags &= ~XFS_ICHGTIME_MOD;
> @@ -524,6 +532,7 @@ xfs_setattr(
> */
>
> if ((flags & XFS_ATTR_DMI) && (mask & ATTR_CTIME)) {
> + inode->i_ctime = iattr->ia_ctime;
> ip->i_d.di_ctime.t_sec = iattr->ia_ctime.tv_sec;
> ip->i_d.di_ctime.t_nsec = iattr->ia_ctime.tv_nsec;
> ip->i_update_core = 1;
> Index: linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/linux-2.6/xfs_xattr.c 2008-07-05 10:02:09.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/linux-2.6/xfs_xattr.c 2008-07-05 12:02:24.000000000 +0200
> @@ -64,7 +64,7 @@ static int
> xfs_xattr_system_set(struct inode *inode, const char *name,
> const void *value, size_t size, int flags)
> {
> - int error, acl;
> + int acl;
>
> acl = xfs_decode_acl(name);
> if (acl < 0)
> @@ -75,10 +75,7 @@ xfs_xattr_system_set(struct inode *inode
> if (!value)
> return xfs_acl_vremove(inode, acl);
>
> - error = xfs_acl_vset(inode, (void *)value, size, acl);
> - if (!error)
> - vn_revalidate(inode);
> - return error;
> + return xfs_acl_vset(inode, (void *)value, size, acl);
> }
>
> static struct xattr_handler xfs_xattr_system_handler = {
> Index: linux-2.6-xfs/fs/xfs/xfs_acl.c
> ===================================================================
> --- linux-2.6-xfs.orig/fs/xfs/xfs_acl.c 2008-07-05 12:26:45.000000000 +0200
> +++ linux-2.6-xfs/fs/xfs/xfs_acl.c 2008-07-05 12:26:53.000000000 +0200
> @@ -734,7 +734,7 @@ xfs_acl_setmode(
> * Copy the u::, g::, o::, and m:: bits from the ACL into the
> * mode. The m:: bits take precedence over the g:: bits.
> */
> - iattr.ia_mask = XFS_AT_MODE;
> + iattr.ia_valid = ATTR_MODE;
> iattr.ia_mode = xfs_vtoi(vp)->i_d.di_mode;
> iattr.ia_mode &= ~(S_IRWXU|S_IRWXG|S_IRWXO);
> ap = acl->acl_entry;
>
next prev parent reply other threads:[~2008-07-15 6:35 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-06-27 15:46 [PATCH 3/3] kill vn_revalidate Christoph Hellwig
2008-07-05 17:21 ` Christoph Hellwig
2008-07-15 6:36 ` Timothy Shimmin [this message]
2008-07-15 12:07 ` Christoph Hellwig
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=487C456D.3010509@sgi.com \
--to=tes@sgi.com \
--cc=hch@lst.de \
--cc=xfs@oss.sgi.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.