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

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

[Lachlan McIlroy]

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

[<ffffffffa0047de7>] xfs_iext_get_ext+0x6c/0xca [xfs]
[<ffffffffa004a3d4>] xfs_iflush_fork+0x1b0/0x3c6 [xfs]
[<ffffffffa004aa3f>] xfs_iflush_int+0x455/0x5a1 [xfs]
[<ffffffffa004b0da>] xfs_iflush+0x229/0x2d6 [xfs]
[<ffffffffa007283c>] xfs_reclaim_inode+0xd8/0x10f [xfs]
[<ffffffffa0072976>] xfs_reclaim_inodes_ag+0x103/0x13e [xfs]
[<ffffffffa00729f3>] xfs_reclaim_inodes+0x42/0x60 [xfs]
[<ffffffffa0072d1b>] xfs_sync_worker+0x30/0x8a [xfs]
[<ffffffffa0073416>] xfssyncd+0x14e/0x1a2 [xfs]
[<ffffffffa00732c8>] ? xfssyncd+0x0/0x1a2 [xfs]
[<ffffffff8104fc70>] kthread+0x49/0x79

I made this change and it passes the load test and XFSQA too.  I'm
not sure if this is indicative of a bigger problem though.

Index: xfs-fix/fs/xfs/xfs_inode.c
===================================================================
--- xfs-fix.orig/fs/xfs/xfs_inode.c
+++ xfs-fix/fs/xfs/xfs_inode.c
@@ -2165,8 +2165,6 @@ xfs_ifree(
  	ip->i_d.di_forkoff = 0;		/* mark the attr fork not in use */
  	ip->i_df.if_ext_max =
  		XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
-	ip->i_d.di_format = XFS_DINODE_FMT_EXTENTS;
-	ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
  	/*
  	 * Bump the generation count so no one will be confused
  	 * by reincarnations of this inode.

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

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

[Eric Sandeen]

Lachlan McIlroy wrote:
> I hit a panic while flushing a reclaimed inode that is fairly
> reproducible under load.

With what testcase?

Thanks,
-Eric

> 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().
> 
> [<ffffffffa0047de7>] xfs_iext_get_ext+0x6c/0xca [xfs]
> [<ffffffffa004a3d4>] xfs_iflush_fork+0x1b0/0x3c6 [xfs]
> [<ffffffffa004aa3f>] xfs_iflush_int+0x455/0x5a1 [xfs]
> [<ffffffffa004b0da>] xfs_iflush+0x229/0x2d6 [xfs]
> [<ffffffffa007283c>] xfs_reclaim_inode+0xd8/0x10f [xfs]
> [<ffffffffa0072976>] xfs_reclaim_inodes_ag+0x103/0x13e [xfs]
> [<ffffffffa00729f3>] xfs_reclaim_inodes+0x42/0x60 [xfs]
> [<ffffffffa0072d1b>] xfs_sync_worker+0x30/0x8a [xfs]
> [<ffffffffa0073416>] xfssyncd+0x14e/0x1a2 [xfs]
> [<ffffffffa00732c8>] ? xfssyncd+0x0/0x1a2 [xfs]
> [<ffffffff8104fc70>] kthread+0x49/0x79
> 
> I made this change and it passes the load test and XFSQA too.  I'm
> not sure if this is indicative of a bigger problem though.
> 
> Index: xfs-fix/fs/xfs/xfs_inode.c
> ===================================================================
> --- xfs-fix.orig/fs/xfs/xfs_inode.c
> +++ xfs-fix/fs/xfs/xfs_inode.c
> @@ -2165,8 +2165,6 @@ xfs_ifree(
>   	ip->i_d.di_forkoff = 0;		/* mark the attr fork not in use */
>   	ip->i_df.if_ext_max =
>   		XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
> -	ip->i_d.di_format = XFS_DINODE_FMT_EXTENTS;
> -	ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
>   	/*
>   	 * Bump the generation count so no one will be confused
>   	 * by reincarnations of this inode.
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

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

[Eric Sandeen]

Lachlan McIlroy wrote:
> Eric Sandeen wrote:
>> Lachlan McIlroy wrote:
>>> I hit a panic while flushing a reclaimed inode that is fairly
>>> reproducible under load.
>> With what testcase?
> Initially with fsstress but then I wrote the extent.c program I
> attached to the next patch I posted.

Ah, ok, thanks.

-Eric

>> Thanks,
>> -Eric

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

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

[Lachlan McIlroy]

Eric Sandeen wrote:
> Lachlan McIlroy wrote:
>> I hit a panic while flushing a reclaimed inode that is fairly
>> reproducible under load.
> 
> With what testcase?
Initially with fsstress but then I wrote the extent.c program I
attached to the next patch I posted.

> 
> Thanks,
> -Eric
> 
>> 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().
>>
>> [<ffffffffa0047de7>] xfs_iext_get_ext+0x6c/0xca [xfs]
>> [<ffffffffa004a3d4>] xfs_iflush_fork+0x1b0/0x3c6 [xfs]
>> [<ffffffffa004aa3f>] xfs_iflush_int+0x455/0x5a1 [xfs]
>> [<ffffffffa004b0da>] xfs_iflush+0x229/0x2d6 [xfs]
>> [<ffffffffa007283c>] xfs_reclaim_inode+0xd8/0x10f [xfs]
>> [<ffffffffa0072976>] xfs_reclaim_inodes_ag+0x103/0x13e [xfs]
>> [<ffffffffa00729f3>] xfs_reclaim_inodes+0x42/0x60 [xfs]
>> [<ffffffffa0072d1b>] xfs_sync_worker+0x30/0x8a [xfs]
>> [<ffffffffa0073416>] xfssyncd+0x14e/0x1a2 [xfs]
>> [<ffffffffa00732c8>] ? xfssyncd+0x0/0x1a2 [xfs]
>> [<ffffffff8104fc70>] kthread+0x49/0x79
>>
>> I made this change and it passes the load test and XFSQA too.  I'm
>> not sure if this is indicative of a bigger problem though.
>>
>> Index: xfs-fix/fs/xfs/xfs_inode.c
>> ===================================================================
>> --- xfs-fix.orig/fs/xfs/xfs_inode.c
>> +++ xfs-fix/fs/xfs/xfs_inode.c
>> @@ -2165,8 +2165,6 @@ xfs_ifree(
>>   	ip->i_d.di_forkoff = 0;		/* mark the attr fork not in use */
>>   	ip->i_df.if_ext_max =
>>   		XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
>> -	ip->i_d.di_format = XFS_DINODE_FMT_EXTENTS;
>> -	ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
>>   	/*
>>   	 * Bump the generation count so no one will be confused
>>   	 * by reincarnations of this inode.
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
>>
> 

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

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

[Felix Blyakher]

On Feb 10, 2009, at 6:26 PM, Lachlan McIlroy wrote:

> I hit a panic while flushing a reclaimed inode that is fairly
> reproducible under load.

What kind of panic was that? Where in xfs_iext_get_ext() did
it panic?

> 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().
>
> [<ffffffffa0047de7>] xfs_iext_get_ext+0x6c/0xca [xfs]

I assume you're running debug xfs, as I can see xfs_iext_get_ext()
only in assert statements.

> [<ffffffffa004a3d4>] xfs_iflush_fork+0x1b0/0x3c6 [xfs]
> [<ffffffffa004aa3f>] xfs_iflush_int+0x455/0x5a1 [xfs]
> [<ffffffffa004b0da>] xfs_iflush+0x229/0x2d6 [xfs]
> [<ffffffffa007283c>] xfs_reclaim_inode+0xd8/0x10f [xfs]
> [<ffffffffa0072976>] xfs_reclaim_inodes_ag+0x103/0x13e [xfs]
> [<ffffffffa00729f3>] xfs_reclaim_inodes+0x42/0x60 [xfs]
> [<ffffffffa0072d1b>] xfs_sync_worker+0x30/0x8a [xfs]
> [<ffffffffa0073416>] xfssyncd+0x14e/0x1a2 [xfs]
> [<ffffffffa00732c8>] ? xfssyncd+0x0/0x1a2 [xfs]
> [<ffffffff8104fc70>] kthread+0x49/0x79
>
> I made this change and it passes the load test and XFSQA too.  I'm
> not sure if this is indicative of a bigger problem though.
>
> Index: xfs-fix/fs/xfs/xfs_inode.c
> ===================================================================
> --- xfs-fix.orig/fs/xfs/xfs_inode.c
> +++ xfs-fix/fs/xfs/xfs_inode.c
> @@ -2165,8 +2165,6 @@ xfs_ifree(
>  	ip->i_d.di_forkoff = 0;		/* mark the attr fork not in use */
>  	ip->i_df.if_ext_max =
>  		XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
> -	ip->i_d.di_format = XFS_DINODE_FMT_EXTENTS;
> -	ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;

So, the idea here is to reset the ip->i_d. It seems strange
to choose XFS_DINODE_FMT_EXTENTS as initializer, and even
more strange how not changing di_aformat could affect your panic.

Just asking the questions at this time.
Felix

>  	/*
>  	 * Bump the generation count so no one will be confused
>  	 * by reincarnations of this inode.
>
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

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

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

[Lachlan McIlroy]

Felix Blyakher wrote:
> 
> On Feb 10, 2009, at 6:26 PM, Lachlan McIlroy wrote:
> 
>> I hit a panic while flushing a reclaimed inode that is fairly
>> reproducible under load.
> 
> What kind of panic was that? Where in xfs_iext_get_ext() did
> it panic?

It was a bad address so I suspect that either ifp->if_u1.if_ext_irec
or ifp->if_u1.if_extents was dereferenced when they have been freed
(or not even allocated).  The correct di_format was XFS_DINODE_FMT_LOCAL
so ifp->if_u1.if_data should have been used.

> 
>> 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().
>>
>> [<ffffffffa0047de7>] xfs_iext_get_ext+0x6c/0xca [xfs]
> 
> I assume you're running debug xfs, as I can see xfs_iext_get_ext()
> only in assert statements.
Yes, debug.

> 
>> [<ffffffffa004a3d4>] xfs_iflush_fork+0x1b0/0x3c6 [xfs]
>> [<ffffffffa004aa3f>] xfs_iflush_int+0x455/0x5a1 [xfs]
>> [<ffffffffa004b0da>] xfs_iflush+0x229/0x2d6 [xfs]
>> [<ffffffffa007283c>] xfs_reclaim_inode+0xd8/0x10f [xfs]
>> [<ffffffffa0072976>] xfs_reclaim_inodes_ag+0x103/0x13e [xfs]
>> [<ffffffffa00729f3>] xfs_reclaim_inodes+0x42/0x60 [xfs]
>> [<ffffffffa0072d1b>] xfs_sync_worker+0x30/0x8a [xfs]
>> [<ffffffffa0073416>] xfssyncd+0x14e/0x1a2 [xfs]
>> [<ffffffffa00732c8>] ? xfssyncd+0x0/0x1a2 [xfs]
>> [<ffffffff8104fc70>] kthread+0x49/0x79
>>
>> I made this change and it passes the load test and XFSQA too.  I'm
>> not sure if this is indicative of a bigger problem though.
>>
>> Index: xfs-fix/fs/xfs/xfs_inode.c
>> ===================================================================
>> --- xfs-fix.orig/fs/xfs/xfs_inode.c
>> +++ xfs-fix/fs/xfs/xfs_inode.c
>> @@ -2165,8 +2165,6 @@ xfs_ifree(
>>      ip->i_d.di_forkoff = 0;        /* mark the attr fork not in use */
>>      ip->i_df.if_ext_max =
>>          XFS_IFORK_DSIZE(ip) / (uint)sizeof(xfs_bmbt_rec_t);
>> -    ip->i_d.di_format = XFS_DINODE_FMT_EXTENTS;
>> -    ip->i_d.di_aformat = XFS_DINODE_FMT_EXTENTS;
> 
> So, the idea here is to reset the ip->i_d. It seems strange
> to choose XFS_DINODE_FMT_EXTENTS as initializer, and even
> more strange how not changing di_aformat could affect your panic.
I figure if one (di_format) is wrong then the other probably is too.

> 
> Just asking the questions at this time.
> Felix
> 
>>      /*
>>       * Bump the generation count so no one will be confused
>>       * by reincarnations of this inode.
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 

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

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

[Dave Chinner]

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

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

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

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

[Lachlan McIlroy]

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

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

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

[Dave Chinner]

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@fromorbit.com

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

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

[Christoph Hellwig]

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?

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

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

[Dave Chinner]

On Sun, Feb 15, 2009 at 02:25:08PM -0500, Christoph Hellwig wrote:
> 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?

I guess we don't need to - I was thinking purely of local format
inodes....

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

No idea, I haven't been able to reproduce it either.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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