Use xfs_inode_clean() in more places.
Signed-off-by: Dave Chinner <[email protected]>
---
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(-)
Index: 2.6.x-xfs-new/fs/xfs/xfs_inode.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_inode.c 2007-11-22 10:33:57.728849000 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_inode.c 2007-11-22 10:33:59.692597965 +1100
@@ -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;
}
Index: 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_vnodeops.c 2007-11-22 10:33:57.732848488 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_vnodeops.c 2007-11-22 10:33:59.696597454 +1100
@@ -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;
/*
Index: 2.6.x-xfs-new/fs/xfs/xfs_inode_item.h
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_inode_item.h 2007-11-22 10:25:23.286572511 +1100
+++ 2.6.x-xfs-new/fs/xfs/xfs_inode_item.h 2007-11-22 10:33:59.696597454 +1100
@@ -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 *);
> +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;
}
On Nov 23 2007 18:02, Christoph Hellwig wrote:
>
>> +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.
I have to disagree, but whatever.
>static inline int xfs_inode_clean(struct xfs_inode *ip)
^ ^
could be bool - and const
>{
> 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;
}
On Fri, 2007-11-23 at 19:16 +0100, Jan Engelhardt wrote:
> 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;
On Nov 23 2007 11:47, Joe Perches wrote:
>On Fri, 2007-11-23 at 19:16 +0100, Jan Engelhardt wrote:
>> 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.
See - the previous cryptic constructs could not even be decoded ;-)
>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.