[PATCH 0/2] xfs: couple of fixes for 3.0-rc2

[PATCH 0/2] xfs: couple of fixes for 3.0-rc2

[Dave Chinner]

These are a couple of fixes for bugs that are candidates for
3.0-rc2.  The first addresses a regression introduced by the dynamic
speculative allocation changes in 2.6.38, and the second is a fix
for a debug-only assert failure I recently discovered while testing
on a selinux enabled system.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/2] xfs: clear inode per-lifetime state when recycling it

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

XFs inodes have several per-lifetime state flags that determine heaviour of the
inode. These state flags are not reset when an inode is reallocated and reused
from the reclaimable state.

This can lead to specualtive preallocation not being truncated away in the
expected manner for local files until the inode is subsequently truncated,
freed or cycles out of the cache. It can also lead to an inode being considered
to be a filestream inode or having been truncated when that is not the case.

Clear the state flags when the inode is recycled to avoid these problems.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_iget.c |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
index cb9b6d1..36467f1 100644
--- a/fs/xfs/xfs_iget.c
+++ b/fs/xfs/xfs_iget.c
@@ -262,7 +262,17 @@ xfs_iget_cache_hit(
 
 		spin_lock(&pag->pag_ici_lock);
 		spin_lock(&ip->i_flags_lock);
-		ip->i_flags &= ~(XFS_IRECLAIMABLE | XFS_IRECLAIM);
+
+		/*
+		 * Clear the relcaim and per-lifetime state flags as we are now
+		 * effectively a new inode and so we need to reset to the
+		 * initial state.
+		 *
+		 * XXX(dgc): should the XFS_ISTALE flag only be cleared here?
+		 */
+		ip->i_flags &= ~(XFS_IRECLAIMABLE | XFS_IRECLAIM |
+				 XFS_IDIRTY_RELEASE | XFS_ITRUNCATED |
+				 XFS_IFILESTREAM);
 		ip->i_flags |= XFS_INEW;
 		__xfs_inode_clear_reclaim_tag(mp, pag, ip);
 		inode->i_state = I_NEW;
-- 
1.7.5.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: clear inode per-lifetime state when recycling it

[Christoph Hellwig]

Looks good.


Reviewed-by: Christoph Hellwig <hch@lst.de>

> +		 * XXX(dgc): should the XFS_ISTALE flag only be cleared here?

I think so.  Right now any iget on a stale inode will clear it, which
is very wrong.  Care to send a separate patch for that?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: clear inode per-lifetime state when recycling it

[Dave Chinner]

On Tue, May 31, 2011 at 04:09:50PM -0400, Christoph Hellwig wrote:
> Looks good.
> 
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> > +		 * XXX(dgc): should the XFS_ISTALE flag only be cleared here?
> 
> I think so.  Right now any iget on a stale inode will clear it, which
> is very wrong.  Care to send a separate patch for that?

However, the di_mode has already been set to zero before it is
marked stale, so any lookup on it without the XFS_IGET_CREATE flag
set will fail. And only inode allocation sets that flag, in which
case we want the XFS_ISTALE flag cleared. We can't have a race with
the XFS_ISTALE flag being set (both inode freeing and allocation
requires the AGI lock), so once it is stale it is protected by the
mode/flag check.

So it seems safe where it is, but it's not exactly obvious why.

Hmmmm. The inode_init_always() failure case does not clear the
XFS_IRECLAIM flag - that seems like a bug as it will prevent the
inode from ever being reclaimed. Indeed, the error handling looks
completely broken - it's like it is assuming the inode has already
been removed from the reclaim list and marked XFS_INEW. Oh, it used
to do exactly that before this commmit:


commit f1f724e4b523d444c5a598d74505aefa3d6844d2
Author: Christoph Hellwig <hch@infradead.org>
Date:   Mon Mar 1 11:30:31 2010 +0000

    xfs: fix locking for inode cache radix tree tag updates

    The radix-tree code requires it's users to serialize tag updates
    against other updates to the tree.  While XFS protects tag updates
    against each other it does not serialize them against updates of the
    tree contents, which can lead to tag corruption.  Fix the inode
    cache to always take pag_ici_lock in exclusive mode when updating
    radix tree tags.

    Signed-off-by: Christoph Hellwig <hch@lst.de>
    Reported-by: Patrick Schreurs <patrick@news-service.com>
    Tested-by: Patrick Schreurs <patrick@news-service.com>
    Signed-off-by: Alex Elder <aelder@sgi.com>

So yes, it needs fixing.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/2] xfs: clear inode per-lifetime state when recycling it

[Alex Elder]

On Tue, 2011-05-31 at 14:20 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> XFs inodes have several per-lifetime state flags that determine heaviour of the
> inode. These state flags are not reset when an inode is reallocated and reused
> from the reclaimable state.
> 
> This can lead to specualtive preallocation not being truncated away in the
> expected manner for local files until the inode is subsequently truncated,
> freed or cycles out of the cache. It can also lead to an inode being considered
> to be a filestream inode or having been truncated when that is not the case.
> 
> Clear the state flags when the inode is recycled to avoid these problems.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

Other than simple typo's, this looks good to me.

Comment below.

Reviewed-by: Alex Elder <aelder@sgi.com>

> ---
>  fs/xfs/xfs_iget.c |   12 +++++++++++-
>  1 files changed, 11 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/xfs/xfs_iget.c b/fs/xfs/xfs_iget.c
> index cb9b6d1..36467f1 100644
> --- a/fs/xfs/xfs_iget.c
> +++ b/fs/xfs/xfs_iget.c
> @@ -262,7 +262,17 @@ xfs_iget_cache_hit(
>  
>  		spin_lock(&pag->pag_ici_lock);
>  		spin_lock(&ip->i_flags_lock);
> -		ip->i_flags &= ~(XFS_IRECLAIMABLE | XFS_IRECLAIM);
> +
> +		/*
> +		 * Clear the relcaim and per-lifetime state flags as we are now

                              reclaim

> +		 * effectively a new inode and so we need to reset to the
> +		 * initial state.
> +		 *
> +		 * XXX(dgc): should the XFS_ISTALE flag only be cleared here?
> +		 */
> +		ip->i_flags &= ~(XFS_IRECLAIMABLE | XFS_IRECLAIM |
> +				 XFS_IDIRTY_RELEASE | XFS_ITRUNCATED |
> +				 XFS_IFILESTREAM);
>  		ip->i_flags |= XFS_INEW;

If you clear XFS_ISTALE here you could simply re-phrase the
above comment to emphasize the "reset to initial state" and
change it to a simple assignment:

 		ip->i_flags = XFS_INEW;

(Though that makes clearing the rest of the flags
implicit, which I think is undesirable from the point
of view of code searching.  XFS_IDIRTY_RELEASE is
already cleared only implicitly.)

Otherwise I was going to suggest you define a symbol
representing the subset of XFS inode flags that are
per-lifetime state flags, so if another one gets
invented along the way it could get added to the set.


>  		__xfs_inode_clear_reclaim_tag(mp, pag, ip);
>  		inode->i_state = I_NEW;



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/2] xfs: prevent bogus assert when trying to remove non-existent attribute

[Dave Chinner]

From: Dave Chinner <dchinner@redhat.com>

If the attribute fork on an inode is in btree format and has
multiple levels (i.e node format rather than leaf format), then a
lookup failure will trigger an assert failure in xfs_da_path_shift
if the flag XFS_DA_OP_OKNOENT is not set. This flag is used to
indicate to the directory btree code that not finding an entry is
not a fatal error. In the case of doing a lookup for a directory
name removal, this is valid as a user cannot insert an arbitrary
name to remove from the directory btree.

However, in the case of the attribute tree, a user has direct
control over the attribute name and can ask for any random name to
be removed without any validation. In this case, fsstress is asking
for a non-existent user.selinux attribute to be removed, and that is
causing xfs_da_path_shift() to fall off the bottom of the tree where
it asserts that a lookup failure is allowed. Because the flag is not
set, we die a horrible death on a debug enable kernel.

Prevent this assert from firing on attribute removes by adding the
op_flag XFS_DA_OP_OKNOENT to atribute removal operations.

Discovered when testing on a SELinux enabled system by fsstress in
test 070 by trying to remove a non-existent user.selinux attribute.

Signed-off-by: Dave Chinner <dchinner@redhat.com>
---
 fs/xfs/xfs_attr.c |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)

diff --git a/fs/xfs/xfs_attr.c b/fs/xfs/xfs_attr.c
index c863753..01d2072 100644
--- a/fs/xfs/xfs_attr.c
+++ b/fs/xfs/xfs_attr.c
@@ -490,6 +490,13 @@ xfs_attr_remove_int(xfs_inode_t *dp, struct xfs_name *name, int flags)
 	args.whichfork = XFS_ATTR_FORK;
 
 	/*
+	 * we have no control over the attribute names that userspace passes us
+	 * to remove, so we have to allow the name lookup prior to attribute
+	 * removal to fail.
+	 */
+	args.op_flags = XFS_DA_OP_OKNOENT;
+
+	/*
 	 * Attach the dquots to the inode.
 	 */
 	error = xfs_qm_dqattach(dp, 0);
-- 
1.7.5.1

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: prevent bogus assert when trying to remove non-existent attribute

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/2] xfs: prevent bogus assert when trying to remove non-existent attribute

[Alex Elder]

On Tue, 2011-05-31 at 14:20 +1000, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
> 
> If the attribute fork on an inode is in btree format and has
> multiple levels (i.e node format rather than leaf format), then a
> lookup failure will trigger an assert failure in xfs_da_path_shift
> if the flag XFS_DA_OP_OKNOENT is not set. This flag is used to
> indicate to the directory btree code that not finding an entry is
> not a fatal error. In the case of doing a lookup for a directory
> name removal, this is valid as a user cannot insert an arbitrary
> name to remove from the directory btree.
> 
> However, in the case of the attribute tree, a user has direct
> control over the attribute name and can ask for any random name to
> be removed without any validation. In this case, fsstress is asking
> for a non-existent user.selinux attribute to be removed, and that is
> causing xfs_da_path_shift() to fall off the bottom of the tree where
> it asserts that a lookup failure is allowed. Because the flag is not
> set, we die a horrible death on a debug enable kernel.
> 
> Prevent this assert from firing on attribute removes by adding the
> op_flag XFS_DA_OP_OKNOENT to atribute removal operations.
> 
> Discovered when testing on a SELinux enabled system by fsstress in
> test 070 by trying to remove a non-existent user.selinux attribute.
> 
> Signed-off-by: Dave Chinner <dchinner@redhat.com>

I have not carefully verified this change, but your
description of the problem was very good so based
on this the change looks right to me.

Signed-off-by: Alex Elder <aelder@sgi.com>



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs