xfs: fix bad hash ordering

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.

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

Index: 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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

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
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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

-Eric

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

...

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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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 <[email protected]> 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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.

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
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

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.

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

Index: 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
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs