xfs
[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: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Date: Sun, 15 Feb 2009 14:25:08 -0500
In-reply-to: <20090212223253.GV8830@disturbed>
References: <49921B3E.8040406@xxxxxxx> <20090211092020.GR8830@disturbed> <49937458.2000500@xxxxxxx> <20090212223253.GV8830@disturbed>
User-agent: Mutt/1.5.18 (2008-05-17)
On Fri, Feb 13, 2009 at 09:32:53AM +1100, Dave Chinner wrote:
> > 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);

Why would we special case directories?

> > 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....

We could, but with the way the function name, comments and how the
unions are set up it would be very confusing to the user. 


Btw, I can't reproduce this issue with the extent.c program and the
invocation from the next patch.  Does it need other parameters to
reproduce?

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