[Top] [All Lists]

Re: [PATCH] Don't reset di_format in xfs_ifree()

To: Lachlan McIlroy <lachlan@xxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
Subject: Re: [PATCH] Don't reset di_format in xfs_ifree()
From: Lachlan McIlroy <lachlan@xxxxxxx>
Date: Thu, 12 Feb 2009 11:59:04 +1100
In-reply-to: <20090211092020.GR8830@disturbed>
Organization: SGI
References: <49921B3E.8040406@xxxxxxx> <20090211092020.GR8830@disturbed>
Reply-to: lachlan@xxxxxxx
User-agent: Thunderbird (X11/20081209)
Dave Chinner wrote:
On Wed, Feb 11, 2009 at 11:26:38AM +1100, Lachlan McIlroy wrote:
I hit a panic while flushing a reclaimed inode that is fairly
reproducible under load.

In xfs_iflush_fork() we're led to believe that there are extents
on this inode but there aren't any.  Actually the inode was a
directory.  I added some debugging to xfs_ifree() and found
that di_format was XFS_DINODE_FMT_LOCAL and got reset to
XFS_DINODE_FMT_EXTENTS and this has confused the code in

Wow. I wonder why we've never seen this before - it's not a new
problem AFAICT.

A freed inode is supposed to have both forks in extent format
with zero extents - it means the fork is empty. Changing it
to local format means that it is not in the expected state
for a subsequent create.

I think the problem may be that the size of the fork has not been
reset to zero, not that format has been changed. If it was in local
format, the truncates prior to freeing would not have done anything
and the size of the data/attr fork would still be non-zero.  Hence
if the fork is then changed to extent format, xfs_iextents_copy()
will be triggered from xfs_iflush_fork() and you'd see something
like the confusion you are seeing.

Hence I think we should be ensuring the fork size is set to zero for
both the attr/data fork when the format is changed, not removing
the change of type....

Yes, I agree.  I just don't have the time to hunt it down.  I see
there's a call to xfs_idestroy_fork() in xfs_ireclaim() for directories
but xfs_ireclaim() gets called after xfs_iflush() in xfs_reclaim_inode().
Might also need something like:

@@ -2445,6 +2447,7 @@ xfs_idestroy_fork(
                        ifp->if_u1.if_data = NULL;
                        ifp->if_real_bytes = 0;
+                       ifp->if_bytes = 0;
        } else if ((ifp->if_flags & XFS_IFEXTENTS) &&
                   ((ifp->if_flags & XFS_IFEXTIREC) ||

<Prev in Thread] Current Thread [Next in Thread>