[PATCH] xfs: fix bad hash ordering

[PATCH] xfs: fix bad hash ordering

[Mark Tinguely]

[-- Attachment #1: xfs-fix-directory-bad-hash-order.patch --]
[-- Type: text/plain, Size: 1087 bytes --]

Fix the fix directory "bad hash ordering" bug introduced in
commit f5ea1100.

Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
A C program that generates this problem can be found at:
 http://oss.sgi.com/archives/xfs/2014-03/msg00373.html

A xfstest for this bug is coming from Hannes Frederic Sowa.

 fs/xfs/xfs_da_btree.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/fs/xfs/xfs_da_btree.c
===================================================================
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -1295,7 +1295,7 @@ xfs_da3_fixhashpath(
 		node = blk->bp->b_addr;
 		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
 		btree = dp->d_ops->node_tree_p(node);
-		if (be32_to_cpu(btree->hashval) == lasthash)
+		if (be32_to_cpu(btree[blk->index].hashval) == lasthash)
 			break;
 		blk->hashval = lasthash;
 		btree[blk->index].hashval = cpu_to_be32(lasthash);


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

Re: [PATCH] xfs: fix bad hash ordering

[Ben Myers]

On Fri, Mar 28, 2014 at 12:33:34PM -0500, Mark Tinguely wrote:
> Fix the fix directory "bad hash ordering" bug introduced in
> commit f5ea1100.
> 
> Reported-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
> A C program that generates this problem can be found at:
>  http://oss.sgi.com/archives/xfs/2014-03/msg00373.html
> 
> A xfstest for this bug is coming from Hannes Frederic Sowa.
> 
>  fs/xfs/xfs_da_btree.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/fs/xfs/xfs_da_btree.c
> ===================================================================
> --- a/fs/xfs/xfs_da_btree.c
> +++ b/fs/xfs/xfs_da_btree.c
> @@ -1295,7 +1295,7 @@ xfs_da3_fixhashpath(
>  		node = blk->bp->b_addr;
>  		dp->d_ops->node_hdr_from_disk(&nodehdr, node);
>  		btree = dp->d_ops->node_tree_p(node);
> -		if (be32_to_cpu(btree->hashval) == lasthash)
> +		if (be32_to_cpu(btree[blk->index].hashval) == lasthash)
>  			break;
>  		blk->hashval = lasthash;
>  		btree[blk->index].hashval = cpu_to_be32(lasthash);

Looks good to me.

Reviewed-by: Ben Myers <bpm@sgi.com>

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

Re: [PATCH] xfs: fix bad hash ordering

[Dave Chinner]

On Fri, Mar 28, 2014 at 12:33:34PM -0500, Mark Tinguely wrote:
> Fix the fix directory "bad hash ordering" bug introduced in
> commit f5ea1100.

The patch looks like it fixes a hashing issue, but I really can't
tell what hashing issue it fixes from the commit message.

I don't know if you've identified the right commit just by looking
at it - to confirm you've quoted the correct hash I have to find it,
parse it, read the diff and try and work out whether it could have
introduced some bug that you haven't described....

.... introduced in commit f5ea110 ("xfs: add CRCs to dir2/da node
blocks") ....

is far more informative, even for the causal reader....

Secondly, reading that commit subject and message in isolation,
it didn't even occur to me that this is a fix for a filesystem
corruption bug. Any distro maintainer reading the commit logs won't
have any idea that they need to pick this up, either.

IOWs, it is extremely important that the commit message for a
filesystem corruption bug fix to be clearly marked as fixing a
corruption bug, explain what the bug is, what they user visible
symptoms are, what the impact of the corruption is, how likely users
are going to be impacted by it, whether is causes permanent loss
of data or not, etc. This doesn't belong in a throw-away comment to
your fix...

> ---
> A C program that generates this problem can be found at:
>  http://oss.sgi.com/archives/xfs/2014-03/msg00373.html
> 
> A xfstest for this bug is coming from Hannes Frederic Sowa.

Can you convert this program to an xfstest yourself so that I can
commit the regression test at the same time I commit an updated
fix?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: fix bad hash ordering

[Eric Sandeen]

On 3/30/14, 6:10 PM, Dave Chinner wrote:
> On Fri, Mar 28, 2014 at 12:33:34PM -0500, Mark Tinguely wrote:
>> Fix the fix directory "bad hash ordering" bug introduced in
>> commit f5ea1100.
> 
> The patch looks like it fixes a hashing issue, but I really can't
> tell what hashing issue it fixes from the commit message.
> 
> I don't know if you've identified the right commit just by looking
> at it - to confirm you've quoted the correct hash I have to find it,
> parse it, read the diff and try and work out whether it could have
> introduced some bug that you haven't described....
> 
> .... introduced in commit f5ea110 ("xfs: add CRCs to dir2/da node
> blocks") ....
> 
> is far more informative, even for the causal reader....
> 
> Secondly, reading that commit subject and message in isolation,
> it didn't even occur to me that this is a fix for a filesystem
> corruption bug. Any distro maintainer reading the commit logs won't
> have any idea that they need to pick this up, either.
> 
> IOWs, it is extremely important that the commit message for a
> filesystem corruption bug fix to be clearly marked as fixing a
> corruption bug, explain what the bug is, what they user visible
> symptoms are, what the impact of the corruption is, how likely users
> are going to be impacted by it, whether is causes permanent loss
> of data or not, etc. This doesn't belong in a throw-away comment to
> your fix...

But thanks a lot for jumping on the bug report & getting the fix out
on the list in short order.  ;)

-Eric

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

Re: [PATCH] xfs: fix bad hash ordering

[Mark Tinguely]

On 03/30/14 19:10, Dave Chinner wrote:
> On Fri, Mar 28, 2014 at 12:33:34PM -0500, Mark Tinguely wrote:
>> Fix the fix directory "bad hash ordering" bug introduced in
>> commit f5ea1100.
>

...

>
>> ---
>> A C program that generates this problem can be found at:
>>   http://oss.sgi.com/archives/xfs/2014-03/msg00373.html
>>
>> A xfstest for this bug is coming from Hannes Frederic Sowa.
>
> Can you convert this program to an xfstest yourself so that I can
> commit the regression test at the same time I commit an updated
> fix?

We narrowed the iterations down to make it a quick test.
I have every confidence that Hannes can generate the test in a timely
manner and I will help in any way possible.

--Mark.

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

Re: [PATCH] xfs: fix bad hash ordering

[Dave Chinner]

On Mon, Mar 31, 2014 at 11:42:46AM -0500, Mark Tinguely wrote:
> On 03/30/14 19:10, Dave Chinner wrote:
> >On Fri, Mar 28, 2014 at 12:33:34PM -0500, Mark Tinguely wrote:
> >>Fix the fix directory "bad hash ordering" bug introduced in
> >>commit f5ea1100.
> >
> 
> ...
> 
> >
> >>---
> >>A C program that generates this problem can be found at:
> >>  http://oss.sgi.com/archives/xfs/2014-03/msg00373.html
> >>
> >>A xfstest for this bug is coming from Hannes Frederic Sowa.
> >
> >Can you convert this program to an xfstest yourself so that I can
> >commit the regression test at the same time I commit an updated
> >fix?
> 
> We narrowed the iterations down to make it a quick test.
> I have every confidence that Hannes can generate the test in a timely
> manner and I will help in any way possible.

Well, it's been over a week now and you're asking me to trust that
someone I don't know and who has never submitted an xfstests before
to do something in a timely manner so we can test a critical bug fix
during a merge window. I'm willing to be pleasently surprised, but
history tells me that people that report bugs rarely follow up with
xfstest cases and it's usually the developer that fixes the bug that
generates the xfstests patch.

So if the xfstests patch doesn't arrive in the next few hours, can
you please do that for us so I can get this sorted out for the merge
window?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: fix bad hash ordering

[Mark Tinguely]

On 03/31/14 16:40, Dave Chinner wrote:
> On Mon, Mar 31, 2014 at 11:42:46AM -0500, Mark Tinguely wrote:
>> On 03/30/14 19:10, Dave Chinner wrote:
>>> On Fri, Mar 28, 2014 at 12:33:34PM -0500, Mark Tinguely wrote:
>>>> Fix the fix directory "bad hash ordering" bug introduced in
>>>> commit f5ea1100.
>>>
>>
>> ...
>>
>>>
>>>> ---
>>>> A C program that generates this problem can be found at:
>>>>   http://oss.sgi.com/archives/xfs/2014-03/msg00373.html
>>>>
>>>> A xfstest for this bug is coming from Hannes Frederic Sowa.
>>>
>>> Can you convert this program to an xfstest yourself so that I can
>>> commit the regression test at the same time I commit an updated
>>> fix?
>>
>> We narrowed the iterations down to make it a quick test.
>> I have every confidence that Hannes can generate the test in a timely
>> manner and I will help in any way possible.
>
> Well, it's been over a week now and you're asking me to trust that
> someone I don't know and who has never submitted an xfstests before
> to do something in a timely manner so we can test a critical bug fix
> during a merge window. I'm willing to be pleasently surprised, but
> history tells me that people that report bugs rarely follow up with
> xfstest cases and it's usually the developer that fixes the bug that
> generates the xfstests patch.
>
> So if the xfstests patch doesn't arrive in the next few hours, can
> you please do that for us so I can get this sorted out for the merge
> window?
>
> Cheers,
>
> Dave.

Dave,

I think we need to take a step back and clear a little confusion here.
There are 2 different directory bugs.

1) Freeing of a already free extent. It presents with the error:
	XFS: Internal error XFS_WANT_CORRUPTED_GOTO at line 16XX of file
	 fs/xfs/xfs_alloc.c.
    Could be a right or a left edge (or both) that is free.

    Morgan Meyers <Morgan.Mears@netapp.com> sent the latest occurrence on
    March 12, but others have been seeing it in the community code in the
    last few mounts. SGI has been seeing it lately with big customers and
    it has occurred off and on for 7-8  years according to our bug
    database.

    It is a nasty bug that can can cause corruption. As I mentioned last
    week in the analysis of Morgan's metadata dump, XFS can allocate the
    same buffer multiple times. In his metadata dump there is a directory
    block and inode clusters that also allocated as user blocks. These
    duplicate allocated blocks are land mines waiting to go off either
    when written to by one owner or when when both allocations are
    removed which causes the XFS_WANT_CORRUPTED_GOTO forced shutdown.

2) Hannes Frederic Sowa found a different directory bug on Thursday,
    March 27. He included a replicator. I bisected the source of the this
    bug on Thursday. Walked the bisected patch on Friday and posted the
    patch. The idea to make a xfstest from the replicator was also made
    on March 28.

    This bug has been only known for 3 business days. I already promised
    that a xfstest will be made. If you need to verify the problem and
    the patch, there already is a replicator.

--Mark.

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

Re: [PATCH] xfs: fix bad hash ordering

[Dave Chinner]

On Mon, Mar 31, 2014 at 09:22:19PM -0500, Mark Tinguely wrote:
> >Well, it's been over a week now and you're asking me to trust that
> >someone I don't know and who has never submitted an xfstests before
> >to do something in a timely manner so we can test a critical bug fix
> >during a merge window. I'm willing to be pleasently surprised, but
> >history tells me that people that report bugs rarely follow up with
> >xfstest cases and it's usually the developer that fixes the bug that
> >generates the xfstests patch.
> >
> >So if the xfstests patch doesn't arrive in the next few hours, can
> >you please do that for us so I can get this sorted out for the merge
> >window?
> >
> >Cheers,
> >
> >Dave.
> 
> Dave,
> 
> I think we need to take a step back and clear a little confusion here.
> There are 2 different directory bugs.
> 
> 1) Freeing of a already free extent. It presents with the error:
> 	XFS: Internal error XFS_WANT_CORRUPTED_GOTO at line 16XX of file
> 	 fs/xfs/xfs_alloc.c.
>    Could be a right or a left edge (or both) that is free.
> 
>    Morgan Meyers <Morgan.Mears@netapp.com> sent the latest occurrence on
>    March 12, but others have been seeing it in the community code in the
>    last few mounts. SGI has been seeing it lately with big customers and
>    it has occurred off and on for 7-8  years according to our bug
>    database.

I fail to see what this has to do with someone providing an xfstests
case for the directory hash regression that was under discussion.

Regardless, I'll take issue with your sweeping generalisation: not
every XFS_WANT_CORRUPTED_GOTO error has the same cause. Indeed, most
of the ones we've seen in the past 7-8 years we've found some kind
of problem with hardware or fixed other bugs that have made it go
away.

The above issue that was reported is - so far - a one of a kind. I
haven't seen any other reports that are even vaguely similar. If SGI
has more customers hitting this problem, then it would be really
nice if SGI could provide that information to the community rather
than complain that they've been seeing it for 8 years. All that
tells us in the community is that you aren't fixing bugs your
customers are hitting and youren't passing them on to people who
might be able to help...

IOWs, if a vendor doesn't have the expertise to find the underlying
problem and they need help tracking down such problems, then they
should report the bugs to the list like end users do.

> 2) Hannes Frederic Sowa found a different directory bug on Thursday,
>    March 27. He included a replicator. I bisected the source of the this
>    bug on Thursday. Walked the bisected patch on Friday and posted the
>    patch. The idea to make a xfstest from the replicator was also made
>    on March 28.
> 
>    This bug has been only known for 3 business days. I already promised
>    that a xfstest will be made. If you need to verify the problem and
>    the patch, there already is a replicator.

The xfstest is *not for me* - it's for every distro and vendor out
there that ships XFS in their product to realise that there's a
serious bug they need fixing, and for them to be able to confirm
that they've fixed it.  I don't ask people to do stuff for my
benefit - I'm perfectly capable of doing random special stuff for
myself - but I will ask for things that are needed for the greater
community.

That's why I asked you to rewrite the commit message to explain what
the cause and impact of problem being fixed was, and why I'm asking
for the regression test to be provided quickly. Both of these things
greatly benefit downstream users of XFS and xfstests, so upstream
processes need to reflect this. Fixing the bug in the upstream tree
is only half the job we need to do...

It's a moot discussion now that the xfstest case has been posted....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[PATCH] xfsprogs: fix directory hash ordering bug

[Mark Tinguely]

[-- Attachment #1: libxfs-fix-directory-bad-hash-order.patch --]
[-- Type: text/plain, Size: 2076 bytes --]

Commit f5ea1100 ("xfs: add CRCs to dir2/da node blocks") introduced
in 3.10 incorrectly converted the btree hash index array pointer in
xfs_da3_fixhashpath(). It resulted in the the current hash always
being compared against the first entry in the btree rather than the
current block index into the btree block's hash entry array. As a
result, it was comparing the wrong hashes, and so could misorder the
entries in the btree.

For most cases, this doesn't cause any problems as it requires hash
collisions to expose the ordering problem. However, when there are
hash collisions within a directory there is a very good probability
that the entries will be ordered incorrectly and that actually
matters when duplicate hashes are placed into or removed from the
btree block hash entry array.

This bug results in an on-disk directory corruption and that results
in directory verifier functions throwing corruption warnings into
the logs. While no data or directory entries are lost, access to
them may be compromised, and attempts to remove entries from a
directory that has suffered from this corruption may result in a
filesystem shutdown.  xfs_repair will fix the directory hash
ordering without data loss occuring.

[dchinner: wrote useful a commit message]

Ported from equivalent kernel commit c88547a8.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 libxfs/xfs_da_btree.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/libxfs/xfs_da_btree.c
===================================================================
--- a/libxfs/xfs_da_btree.c
+++ b/libxfs/xfs_da_btree.c
@@ -1313,7 +1313,7 @@ xfs_da3_fixhashpath(
 		node = blk->bp->b_addr;
 		xfs_da3_node_hdr_from_disk(&nodehdr, node);
 		btree = xfs_da3_node_tree_p(node);
-		if (be32_to_cpu(btree->hashval) == lasthash)
+		if (be32_to_cpu(btree[blk->index]->hashval) == lasthash)
 			break;
 		blk->hashval = lasthash;
 		btree[blk->index].hashval = cpu_to_be32(lasthash);


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

Re: [PATCH] xfsprogs: fix directory hash ordering bug

[Dave Chinner]

On Mon, Apr 07, 2014 at 02:00:34PM -0500, Mark Tinguely wrote:
> Commit f5ea1100 ("xfs: add CRCs to dir2/da node blocks") introduced
> in 3.10 incorrectly converted the btree hash index array pointer in
> xfs_da3_fixhashpath(). It resulted in the the current hash always
> being compared against the first entry in the btree rather than the
> current block index into the btree block's hash entry array. As a
> result, it was comparing the wrong hashes, and so could misorder the
> entries in the btree.
> 
> For most cases, this doesn't cause any problems as it requires hash
> collisions to expose the ordering problem. However, when there are
> hash collisions within a directory there is a very good probability
> that the entries will be ordered incorrectly and that actually
> matters when duplicate hashes are placed into or removed from the
> btree block hash entry array.
> 
> This bug results in an on-disk directory corruption and that results
> in directory verifier functions throwing corruption warnings into
> the logs. While no data or directory entries are lost, access to
> them may be compromised, and attempts to remove entries from a
> directory that has suffered from this corruption may result in a
> filesystem shutdown.  xfs_repair will fix the directory hash
> ordering without data loss occuring.
> 
> [dchinner: wrote useful a commit message]
> 
> Ported from equivalent kernel commit c88547a8.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  libxfs/xfs_da_btree.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/libxfs/xfs_da_btree.c
> ===================================================================
> --- a/libxfs/xfs_da_btree.c
> +++ b/libxfs/xfs_da_btree.c
> @@ -1313,7 +1313,7 @@ xfs_da3_fixhashpath(
>  		node = blk->bp->b_addr;
>  		xfs_da3_node_hdr_from_disk(&nodehdr, node);
>  		btree = xfs_da3_node_tree_p(node);
> -		if (be32_to_cpu(btree->hashval) == lasthash)
> +		if (be32_to_cpu(btree[blk->index]->hashval) == lasthash)
                                                 ^^
xfs_da_btree.c: In function 'xfs_da3_fixhashpath':
xfs_da_btree.c:1316:7: error: invalid type argument of '->' (have 'struct xfs_da_node_entry')
xfs_da_btree.c:1316:7: error: invalid type argument of '->' (have 'struct xfs_da_node_entry')
xfs_da_btree.c:1316:7: error: invalid type argument of '->' (have 'struct xfs_da_node_entry')
xfs_da_btree.c:1316:7: error: invalid type argument of '->' (have 'struct xfs_da_node_entry')
xfs_da_btree.c:1316:7: error: invalid type argument of '->' (have 'struct xfs_da_node_entry')
xfs_da_btree.c:1316:7: error: invalid type argument of '->' (have 'struct xfs_da_node_entry')

I've fixed the copy I've got in my testing tree, but please be more
careful to test patches before you send them for review in future.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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