Clean up open coded inode dirty checks
PATCH 9/9 Clean up open coded inode dirty checks
Use xfs_inode_clean() in more places.
--- fs/xfs/xfs_inode.c | 27 +++++---------------------- fs/xfs/xfs_inode_item.h | 8 ++++++++ fs/xfs/xfs_vnodeops.c | 4 +--- 3 files changed, 14 insertions(+), 25 deletions(-)
fs/xfs/xfs_inode.c +5 -22
@@ -2158,13 +2158,6 @@ xfs_iunlink_remove(return 0 ; } -STATIC_INLINE int xfs_inode_clean(xfs_inode_t *ip) -{ -return (((ip->i_itemp ==NULL ) || - !(ip->i_itemp->ili_format.ilf_fields &XFS_ILOG_ALL )) && - (ip->i_update_core ==0 )); -} -/* lookup all the inodes in the cluster */ STATIC int xfs_icluster_lookup(
@@ -3067,7 +3060,6 @@ xfs_iflush_cluster(int ilist_size;xfs_inode_t **ilist;xfs_inode_t *iq; -xfs_inode_log_item_t *iip;int nr_found;int clcount =0 ;int bufwasdelwri;
@@ -3094,13 +3086,8 @@ xfs_iflush_cluster(* is a candidate for flushing. These checks will be repeated * later after the appropriate locks are acquired. */ - iip = iq->i_itemp; -if ((iq->i_update_core ==0 ) && - ((iip ==NULL ) || - !(iip->ili_format.ilf_fields &XFS_ILOG_ALL )) && -xfs_ipincount (iq) ==0 ) { +if (xfs_inode_clean (iq) &&xfs_ipincount (iq) ==0 )continue ; - }/* * Try to get locks. If any are unavailable or it is pinned,
@@ -3123,10 +3110,8 @@ xfs_iflush_cluster(* arriving here means that this inode can be flushed. First * re-check that it's dirty before flushing. */ - iip = iq->i_itemp; -if ((iq->i_update_core !=0 ) || ((iip !=NULL ) && - (iip->ili_format.ilf_fields &XFS_ILOG_ALL ))) { -int error; +if (!xfs_inode_clean (iq)) { +int error; error =xfs_iflush_int (iq, bp);if (error) {xfs_iunlock (iq,XFS_ILOCK_SHARED );
@@ -3230,8 +3215,7 @@ xfs_iflush(* If the inode isn't dirty, then just release the inode * flush lock and do nothing. */ -if ((ip->i_update_core ==0 ) && - ((iip ==NULL ) || !(iip->ili_format.ilf_fields &XFS_ILOG_ALL ))) { +if (xfs_inode_clean (ip)) {ASSERT ((iip !=NULL ) ? !(iip->ili_item.li_flags &XFS_LI_IN_AIL ) :1 );xfs_ifunlock (ip);
@@ -3398,8 +3382,7 @@ xfs_iflush_int(* If the inode isn't dirty, then just release the inode * flush lock and do nothing. */ -if ((ip->i_update_core ==0 ) && - ((iip ==NULL ) || !(iip->ili_format.ilf_fields &XFS_ILOG_ALL ))) { +if (xfs_inode_clean (ip)) {xfs_ifunlock (ip);return 0 ; }
fs/xfs/xfs_vnodeops.c +1 -3
@@ -3532,7 +3532,6 @@ xfs_inode_flush( int flags) {xfs_mount_t *mp = ip->i_mount; -xfs_inode_log_item_t *iip = ip->i_itemp;int error =0 ;if (XFS_FORCED_SHUTDOWN (mp))
@@ -3542,8 +3541,7 @@ xfs_inode_flush(* Bypass inodes which have already been cleaned by * the inode flush clustering code inside xfs_iflush */ -if ((ip->i_update_core ==0 ) && - ((iip ==NULL ) || !(iip->ili_format.ilf_fields &XFS_ILOG_ALL ))) +if (xfs_inode_clean (ip))return 0 ;/*
fs/xfs/xfs_inode_item.h +8 -0
@@ -168,6 +168,14 @@ static inline int xfs_ilog_fext(int w)return (w ==XFS_DATA_FORK ?XFS_ILOG_DEXT :XFS_ILOG_AEXT ); } +STATIC_INLINE int xfs_inode_clean(xfs_inode_t *ip) +{ +return (((ip->i_itemp ==NULL ) || + !(ip->i_itemp->ili_format.ilf_fields &XFS_ILOG_ALL )) && + (ip->i_update_core ==0 )); +} + +#ifdef __KERNEL__extern void xfs_inode_item_init (struct xfs_inode *,struct xfs_mount *);
fs/xfs/xfs_inode_item.h static inline int xfs_ilog_fext(int w) · line 176
Quoted 6 lines
fs/xfs/xfs_inode_item.h +6
-0+STATIC_INLINE int xfs_inode_clean(xfs_inode_t *ip) +{ +return (((ip->i_itemp ==NULL ) || + !(ip->i_itemp->ili_format.ilf_fields &XFS_ILOG_ALL )) && + (ip->i_update_core ==0 )); +}
Can we please get rid of this useless STATIC_INLINE junk? It's really hurting my eyes. As does to a lesser extent the verbose style of this function. This should be something like:
static inline int xfs_inode_clean(struct xfs_inode *ip)
{
return (!ip->i_itemp ||
!(ip->i_itemp->ili_format.ilf_fields & XFS_ILOG_ALL)) &&
!ip->i_update_core;
}Quoted 11 lines · Christoph Hellwig
fs/xfs/xfs_inode_item.h static inline int xfs_ilog_fext(int w) · line 176
Quoted 7 lines
fs/xfs/xfs_inode_item.h +6
-0+STATIC_INLINE int xfs_inode_clean(xfs_inode_t *ip) +{ +return (((ip->i_itemp ==NULL ) || + !(ip->i_itemp->ili_format.ilf_fields &XFS_ILOG_ALL )) && + (ip->i_update_core ==0 )); +}Can we please get rid of this useless
STATIC_INLINEjunk? It's really hurting my eyes.
Quoted 2 lines
As does to a lesser extent the verbose style of this function.
I have to disagree, but whatever.
Quoted 1 line
static inline int
xfs_inode_clean(struct xfs_inode *ip)
^ ^
could be bool - and const
Quoted 5 lines
{
return (!ip->i_itemp ||
!(ip->i_itemp->ili_format.ilf_fields & XFS_ILOG_ALL)) &&
!ip->i_update_core;
}Perhaps for greater readability:
static inline bool xfs_inode_clean(const struct xfs_inode *ip)
{
if (ip->i_itemp == NULL)
return true;
if (!(ip->i_itemp->ili_format.ilf_fields & XFS_ILOG_ALL) &&
ip->i_update_core == NULL)
return true;
return false;
}Quoted 9 lines · Jan Engelhardt
static inline bool
xfs_inode_clean(const struct xfs_inode *ip){ if (ip->i_itemp == NULL) return true; if (!(ip->i_itemp->ili_format.ilf_fields & XFS_ILOG_ALL) && ip->i_update_core == NULL) return true; return false; }
Your code changed the test.
xfs_inode.i_update_core is an unsigned char.
I believe reordering the tests to avoid a possibly unnecessary dereference is better.
if (ip->i_update_core) return false; if (!ip->i_itemp) return true; return ip->i_itemp->ili_format.ilf_fields & XFS_ILOG_ALL;
Quoted 11 lines · Joe Perches
Quoted 9 lines · Jan Engelhardt
static inline bool
xfs_inode_clean(const struct xfs_inode *ip){ if (ip->i_itemp == NULL) return true; if (!(ip->i_itemp->ili_format.ilf_fields & XFS_ILOG_ALL) && ip->i_update_core == NULL) return true; return false; }
Quoted 1 line
Your code changed the test.
See - the previous cryptic constructs could not even be decoded ;-)
Quoted 10 lines
xfs_inode.i_update_core is an unsigned char.
I believe reordering the tests to avoid a possibly unnecessary dereference is better.
if (ip->i_update_core) return false; if (!ip->i_itemp) return true; return ip->i_itemp->ili_format.ilf_fields & XFS_ILOG_ALL;
Yeah, something like that.
Note: the function SHOULD return bool for this, to quash the ilf_fields & XFS_ILOG_ALL into 0/1.