xfs
[Top] [All Lists]

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

To: Lachlan McIlroy <lachlan@xxxxxxx>
Subject: Re: [PATCH] Don't reset di_format in xfs_ifree()
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 13 Feb 2009 09:32:53 +1100
Cc: xfs-oss <xfs@xxxxxxxxxxx>
In-reply-to: <49937458.2000500@xxxxxxx>
Mail-followup-to: Lachlan McIlroy <lachlan@xxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>
References: <49921B3E.8040406@xxxxxxx> <20090211092020.GR8830@disturbed> <49937458.2000500@xxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
On Thu, Feb 12, 2009 at 11:59:04AM +1100, Lachlan McIlroy wrote:
> 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
>>> xfs_iflush_fork().
....
>> 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().

I suspect it should be in xfs_inactive() if we are in local format.
This is what happens with the attribute fork.  I think that is where
we need something like:

        if ((ip->i_d.di_mode & S_IFMT) == S_IFDIR &&
            ip->i_d.di_nextents == 0)
                xfs_idestroy_fork(ip, XFS_DATA_FORK);

> Might also need something like:
>
> @@ -2445,6 +2447,7 @@ xfs_idestroy_fork(
>                         kmem_free(ifp->if_u1.if_data);
>                         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) ||

Looking at that, the whole if (local) {} else if (extent/btree)
code could probably be replaced with a single call to
xfs_iext_destroy() as it does the cleanup correctly in both cases,
anyway....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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