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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox