xfs
[Top] [All Lists]

Re: [PATCH] xfs: recalculate leaf entry pointer after compacting a dir2

To: Eric Sandeen <sandeen@xxxxxxxxxx>
Subject: Re: [PATCH] xfs: recalculate leaf entry pointer after compacting a dir2 block
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Fri, 11 Jan 2013 09:47:09 +1100
Cc: xfs-oss <xfs@xxxxxxxxxxx>, Dave Jones <davej@xxxxxxxxxx>, Dave Chinner <dchinner@xxxxxxxxxx>
In-reply-to: <50EEEF4C.8040801@xxxxxxxxxx>
References: <50EEEF4C.8040801@xxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
On Thu, Jan 10, 2013 at 10:41:48AM -0600, Eric Sandeen wrote:
> Dave Jones hit this assert when doing a compile on recent git, with
> CONFIG_XFS_DEBUG enabled:
> 
> XFS: Assertion failed: (char *)dup - (char *)hdr == 
> be16_to_cpu(*xfs_dir2_data_unused_tag_p(dup)), file: fs/xfs/xfs_dir2_data.c, 
> line: 828
> 
> Upon further digging, the tag found by xfs_dir2_data_unused_tag_p(dup)
> contained "2" and not the proper offset, and I found that this value was
> changed after the memmoves under "Use a stale leaf for our new entry."
> in xfs_dir2_block_addname(), i.e.
> 
>                         memmove(&blp[mid + 1], &blp[mid],
>                                 (highstale - mid) * sizeof(*blp));
> 
> overwrote it.
> 
> What has happened is that the previous call to xfs_dir2_block_compact()
> has rearranged things; it changes btp->count as well as the
> blp array.  So after we make that call, we must recalculate the
> proper pointer to the leaf entries by making another call to
> xfs_dir2_block_leaf_p().

Perhaps a bit of ascii art to explain it for other reviewers to save
them having to spend a long time trying to understand the dir2 code?

Pre-compact:

 dup    +   unused data pointer - size 72 bytes at offset 3560
        |
        |
 blp    +   leaf entry [0] - good - at offset 3632
        +   leaf entry [1] - good
        +   leaf entry [2] - stale
        +   leaf entry [3] - good
        +   leaf entry [4] - good
        +   leaf entry [5] - stale
        +   leaf entry [6] - good
        .....
        +   leaf entry [n] - stale
        +   leaf entry [n+1] - good
        +   block tail - points to offset 3632

Post compact (removes all but the highest stale leaf entry, adds the
free space to the last unused data region):

 dup    +   unused data pointer - size 112 bytes at offset 3560
        |
        |
 blp    |   points into unused region, not to start of leaf array
        |
        |
        +   leaf entry [0] - good - at offset 3672
        +   leaf entry [1] - good
        +   leaf entry [2] - good
        +   leaf entry [3] - good
        .....
        +   leaf entry [n] - stale
        +   leaf entry [n+1] - good
        +   block tail - points to offset 3672

So as you can see, blp needs to be updated to point at offset 3672...

> Dave provided a metadump image which led to a simple reproducer
> (create a particular filename in the affected directory) and this
> resolves the testcase as well as the bug on his live system.

Thanks to Dave for making this easy to find :)

> Thanks also to dchinner for looking at this one with me.
> 
> Signed-off-by: Eric Sandeen <sandeen@xxxxxxxxxx>
> Tested-by: Dave Jones <davej@xxxxxxxxxx>

A simple fix for a complex problem. Nice work, Eric.

Reviewed-by: Dave Chinner <dchinner@xxxxxxxxxx>

FWIW, it's worth mentioning that the dir2 block verifier initially
detected this tag corruption on Dave Jones' machine. The verifier
didn't pinpoint the cause of the problem, but they definitely caught
the result (corrupt tags) when the buffer was being submitted to
disk and so that gave us the initial heads-up there was something
wrong in this code....

Cheers,

Dave.
-- 
Dave Chinner
david@xxxxxxxxxxxxx

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