[PATCH] xfs: fix node forward in xfs_node_toosmall

[PATCH] xfs: fix node forward in xfs_node_toosmall

[Mark Tinguely]

[-- Attachment #1: xfs-fix-xfs_da3_node_toosmall-test.patch --]
[-- Type: text/plain, Size: 1670 bytes --]

Commit f5ea1100 cleans up the disk to host conversions for
node directory entries, but because a variable is reused in
xfs_node_toosmall() the next node is not correctly found.
If the original node is small enough (<= 3/8 of the node size),
this change may incorrectly cause a node collapse when it should
not. That will cause an assert in xfstest generic/319:

   Assertion failed: first <= last && last < BBTOB(bp->b_length),
   file: /root/newest/xfs/fs/xfs/xfs_trans_buf.c, line: 569

Keep the original node header to get the correct forward node.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 fs/xfs/xfs_da_btree.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_da_btree.c
===================================================================
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -1163,6 +1163,7 @@ xfs_da3_node_toosmall(
 	xfs_dablk_t		blkno;
 	struct xfs_buf		*bp;
 	struct xfs_da3_icnode_hdr nodehdr;
+	struct xfs_da3_icnode_hdr firstnhdr;
 	int			count;
 	int			forward;
 	int			error;
@@ -1221,13 +1222,14 @@ xfs_da3_node_toosmall(
 	count -= state->node_ents >> 2;
 	count -= nodehdr.count;
 
+	firstnhdr = nodehdr;
 	/* start with smaller blk num */
 	forward = nodehdr.forw < nodehdr.back;
 	for (i = 0; i < 2; forward = !forward, i++) {
 		if (forward)
-			blkno = nodehdr.forw;
+			blkno = firstnhdr.forw;
 		else
-			blkno = nodehdr.back;
+			blkno = firstnhdr.back;
 		if (blkno == 0)
 			continue;
 		error = xfs_da3_node_read(state->args->trans, state->args->dp,


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

Re: [PATCH] xfs: fix node forward in xfs_node_toosmall

[Dave Chinner]

On Fri, Sep 20, 2013 at 05:05:08PM -0500, Mark Tinguely wrote:
> Commit f5ea1100 cleans up the disk to host conversions for
> node directory entries, but because a variable is reused in
> xfs_node_toosmall() the next node is not correctly found.
> If the original node is small enough (<= 3/8 of the node size),
> this change may incorrectly cause a node collapse when it should
> not. That will cause an assert in xfstest generic/319:
> 
>    Assertion failed: first <= last && last < BBTOB(bp->b_length),
>    file: /root/newest/xfs/fs/xfs/xfs_trans_buf.c, line: 569
> 
> Keep the original node header to get the correct forward node.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  fs/xfs/xfs_da_btree.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: b/fs/xfs/xfs_da_btree.c
> ===================================================================
> --- a/fs/xfs/xfs_da_btree.c
> +++ b/fs/xfs/xfs_da_btree.c
> @@ -1163,6 +1163,7 @@ xfs_da3_node_toosmall(
>  	xfs_dablk_t		blkno;
>  	struct xfs_buf		*bp;
>  	struct xfs_da3_icnode_hdr nodehdr;
> +	struct xfs_da3_icnode_hdr firstnhdr;
>  	int			count;
>  	int			forward;
>  	int			error;
> @@ -1221,13 +1222,14 @@ xfs_da3_node_toosmall(
>  	count -= state->node_ents >> 2;
>  	count -= nodehdr.count;
>  
> +	firstnhdr = nodehdr;
>  	/* start with smaller blk num */
>  	forward = nodehdr.forw < nodehdr.back;
>  	for (i = 0; i < 2; forward = !forward, i++) {
>  		if (forward)
> -			blkno = nodehdr.forw;
> +			blkno = firstnhdr.forw;
>  		else
> -			blkno = nodehdr.back;
> +			blkno = firstnhdr.back;
>  		if (blkno == 0)
>  			continue;
>  		error = xfs_da3_node_read(state->args->trans, state->args->dp,

Yes, that definitely a bug, but I think that the change doesn't
scope correctly. The original node header doesn't need to be saved
like this - the node header decoded in the loop needs a loop-scope
variable. i.e.:

	/* start with smaller blk num */
	forward = nodehdr.forw < nodehdr.back;
	for (i = 0; i < 2; forward = !forward, i++) {
+		struct xfs_da3_icnode_hdr thdr;
+
		if (forward)
			blkno = nodehdr.forw;
		else
			blkno = nodehdr.back;
		if (blkno == 0)
			continue;
		error = xfs_da3_node_read(state->args->trans, state->args->dp,
					blkno, -1, &bp, state->args->whichfork);
		if (error)
			return(error);

		node = bp->b_addr;
-		xfs_da3_node_hdr_from_disk(&nodehdr, node);
+		xfs_da3_node_hdr_from_disk(&thdr, node);
		xfs_trans_brelse(state->args->trans, bp);

-		if (count - nodehdr.count >= 0)
+		if (count - thdr.count >= 0)
			break;  /* fits with at least 25% to spare */
	}

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 node forward in xfs_node_toosmall

[Mark Tinguely]

On 09/22/13 19:08, Dave Chinner wrote:
> On Fri, Sep 20, 2013 at 05:05:08PM -0500, Mark Tinguely wrote:
>> Commit f5ea1100 cleans up the disk to host conversions for
>> node directory entries, but because a variable is reused in
>> xfs_node_toosmall() the next node is not correctly found.
>> If the original node is small enough (<= 3/8 of the node size),
>> this change may incorrectly cause a node collapse when it should
>> not. That will cause an assert in xfstest generic/319:
>>
>>     Assertion failed: first<= last&&  last<  BBTOB(bp->b_length),
>>     file: /root/newest/xfs/fs/xfs/xfs_trans_buf.c, line: 569
>>
>> Keep the original node header to get the correct forward node.
>>
>> Signed-off-by: Mark Tinguely<tinguely@sgi.com>
>> ---

...

> Yes, that definitely a bug, but I think that the change doesn't
> scope correctly. The original node header doesn't need to be saved
> like this - the node header decoded in the loop needs a loop-scope
> variable. i.e.:
>
> 	/* start with smaller blk num */
> 	forward = nodehdr.forw<  nodehdr.back;
> 	for (i = 0; i<  2; forward = !forward, i++) {
> +		struct xfs_da3_icnode_hdr thdr;
> +
> 		if (forward)
> 			blkno = nodehdr.forw;
> 		else
> 			blkno = nodehdr.back;
> 		if (blkno == 0)
> 			continue;
> 		error = xfs_da3_node_read(state->args->trans, state->args->dp,
> 					blkno, -1,&bp, state->args->whichfork);
> 		if (error)
> 			return(error);
>
> 		node = bp->b_addr;
> -		xfs_da3_node_hdr_from_disk(&nodehdr, node);
> +		xfs_da3_node_hdr_from_disk(&thdr, node);
> 		xfs_trans_brelse(state->args->trans, bp);
>
> -		if (count - nodehdr.count>= 0)
> +		if (count - thdr.count>= 0)
> 			break;  /* fits with at least 25% to spare */
> 	}
>
> Cheers,
>
> Dave.

Okay thanks.

--Mark.

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

[PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Mark Tinguely]

[-- Attachment #1: v2-xfs-fix-xfs_da3_node_toosmall-test.patch --]
[-- Type: text/plain, Size: 1680 bytes --]

Commit f5ea1100 cleans up the disk to host conversions for
node directory entries, but because a variable is reused in
xfs_node_toosmall() the next node is not correctly found.
If the original node is small enough (<= 3/8 of the node size),
this change may incorrectly cause a node collapse when it should
not. That will cause an assert in xfstest generic/319:

   Assertion failed: first <= last && last < BBTOB(bp->b_length),
   file: /root/newest/xfs/fs/xfs/xfs_trans_buf.c, line: 569

Keep the original node header to get the correct forward node.

Signed-off-by: Mark Tinguely <tinguely@sgi.com>
---
 v2 -> Dave's local variable approach.
    -> send to -stable this bug is in 3.10 and 3.11

 fs/xfs/xfs_da_btree.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

Index: b/fs/xfs/xfs_da_btree.c
===================================================================
--- a/fs/xfs/xfs_da_btree.c
+++ b/fs/xfs/xfs_da_btree.c
@@ -1224,6 +1224,7 @@ xfs_da3_node_toosmall(
 	/* start with smaller blk num */
 	forward = nodehdr.forw < nodehdr.back;
 	for (i = 0; i < 2; forward = !forward, i++) {
+		struct xfs_da3_icnode_hdr thdr; 
 		if (forward)
 			blkno = nodehdr.forw;
 		else
@@ -1236,10 +1237,10 @@ xfs_da3_node_toosmall(
 			return(error);
 
 		node = bp->b_addr;
-		xfs_da3_node_hdr_from_disk(&nodehdr, node);
+		xfs_da3_node_hdr_from_disk(&thdr, node);
 		xfs_trans_brelse(state->args->trans, bp);
 
-		if (count - nodehdr.count >= 0)
+		if (count - thdr.count >= 0)
 			break;	/* fits with at least 25% to spare */
 	}
 	if (i >= 2) {


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

Re: [PATCH] xfs: fix node forward in xfs_node_toosmall

[Michael L. Semon]

On 09/20/2013 06:05 PM, Mark Tinguely wrote:
> Commit f5ea1100 cleans up the disk to host conversions for
> node directory entries, but because a variable is reused in
> xfs_node_toosmall() the next node is not correctly found.
> If the original node is small enough (<= 3/8 of the node size),
> this change may incorrectly cause a node collapse when it should
> not. That will cause an assert in xfstest generic/319:
> 
>    Assertion failed: first <= last && last < BBTOB(bp->b_length),
>    file: /root/newest/xfs/fs/xfs/xfs_trans_buf.c, line: 569
> 
> Keep the original node header to get the correct forward node.

This works for xfstests generic/319 on 32-bit x86.  The xfstests 
run was okay, with a lockdep in the middle and a crash in xfs/300. 
Neither the lockdep nor the crash could reproduced by running 
individual tests one by one.

generic/319 itself fails only because it couldn't find its 
_cleanup routine.  I'm not sure if I'm using v1 or v2 of 
generic/319.

The kernel is set to the commit just before the latest aio-next 
merge, which is causing severe problems here.  xfsprogs was the 
latest production xfsprogs because my copy of xfstests is having 
trouble checking v4 XFS filesystems otherwise.

Will test xfs_node_toosmall patch v2 once I get home.

Thanks!

Michael

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

Re: [PATCH] xfs: fix node forward in xfs_node_toosmall

[Mark Tinguely]

On 09/23/13 16:34, Michael L. Semon wrote:
> On 09/20/2013 06:05 PM, Mark Tinguely wrote:
>> Commit f5ea1100 cleans up the disk to host conversions for
>> node directory entries, but because a variable is reused in
>> xfs_node_toosmall() the next node is not correctly found.
>> If the original node is small enough (<= 3/8 of the node size),
>> this change may incorrectly cause a node collapse when it should
>> not. That will cause an assert in xfstest generic/319:
>>
>>     Assertion failed: first<= last&&  last<  BBTOB(bp->b_length),
>>     file: /root/newest/xfs/fs/xfs/xfs_trans_buf.c, line: 569
>>
>> Keep the original node header to get the correct forward node.
>
> This works for xfstests generic/319 on 32-bit x86.  The xfstests
> run was okay, with a lockdep in the middle and a crash in xfs/300.
> Neither the lockdep nor the crash could reproduced by running
> individual tests one by one.
>
> generic/319 itself fails only because it couldn't find its
> _cleanup routine.  I'm not sure if I'm using v1 or v2 of
> generic/319.
>
> The kernel is set to the commit just before the latest aio-next
> merge, which is causing severe problems here.  xfsprogs was the
> latest production xfsprogs because my copy of xfstests is having
> trouble checking v4 XFS filesystems otherwise.
>
> Will test xfs_node_toosmall patch v2 once I get home.
>
> Thanks!
>
> Michael
>

319 v1 had the cleanup and removed in v2 because of feedback.
IMO, it should be there even basically empty.

--Mark.

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Dave Chinner]

On Mon, Sep 23, 2013 at 12:18:58PM -0500, Mark Tinguely wrote:
> Commit f5ea1100 cleans up the disk to host conversions for
> node directory entries, but because a variable is reused in
> xfs_node_toosmall() the next node is not correctly found.
> If the original node is small enough (<= 3/8 of the node size),
> this change may incorrectly cause a node collapse when it should
> not.

The comment about the size of the node triggering a collapse is
irrelevant - nodes always collapse at that given size. What this
doesn't tell us is why the crash occurs - "the next node is not
correctly found" is not particularly obvious, and would require
quite a bit of code reading to work out from first principles a
couple of years down the track.

The commit message should be more precise and describe what the
underlying cause of the failure was. i.e. that the node is finding itself as the merge
candidate because we go forward, overwrite the pointers and the new
block's backward sibling is the original block which is where we end
up on teh second loop.  And vice versa if we go backwards first...

Also, the "next node" is correctly termed a "sibling", and it's
either the forwards or backwards sibling, not the "next" sibling as
the direction of movement is important. So perhaps this
is better written as:

"When a node is considered for a merge with a sibling, it overwrites
the sibling pointers of the original node with the sibling's
pointers. This leads to loop considering the original node as a
merge candidate with itself in the second pass, and so it
incorrectly determines a merge should occur."

> That will cause an assert in xfstest generic/319:
> 
>    Assertion failed: first <= last && last < BBTOB(bp->b_length),
>    file: /root/newest/xfs/fs/xfs/xfs_trans_buf.c, line: 569
> 
> Keep the original node header to get the correct forward node.
> 
> Signed-off-by: Mark Tinguely <tinguely@sgi.com>
> ---
>  v2 -> Dave's local variable approach.
>     -> send to -stable this bug is in 3.10 and 3.11

The patch title not include "v2". That goes in the "[PATCH v2]"
prefix that gets stripped before commit so that commit messages
don't have patch version numbers in them....

Otherwise the code looks good.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Mark Tinguely]

On 09/23/13 18:48, Dave Chinner wrote:
> On Mon, Sep 23, 2013 at 12:18:58PM -0500, Mark Tinguely wrote:
>> Commit f5ea1100 cleans up the disk to host conversions for
>> node directory entries, but because a variable is reused in
>> xfs_node_toosmall() the next node is not correctly found.
>> If the original node is small enough (<= 3/8 of the node size),
>> this change may incorrectly cause a node collapse when it should
>> not.
>
> The comment about the size of the node triggering a collapse is
> irrelevant - nodes always collapse at that given size. What this
> doesn't tell us is why the crash occurs - "the next node is not
> correctly found" is not particularly obvious, and would require
> quite a bit of code reading to work out from first principles a
> couple of years down the track.
>
> The commit message should be more precise and describe what the
> underlying cause of the failure was. i.e. that the node is finding itself as the merge
> candidate because we go forward, overwrite the pointers and the new
> block's backward sibling is the original block which is where we end
> up on teh second loop.  And vice versa if we go backwards first...
>
> Also, the "next node" is correctly termed a "sibling", and it's
> either the forwards or backwards sibling, not the "next" sibling as
> the direction of movement is important. So perhaps this
> is better written as:
>
> "When a node is considered for a merge with a sibling, it overwrites
> the sibling pointers of the original node with the sibling's
> pointers. This leads to loop considering the original node as a
> merge candidate with itself in the second pass, and so it
> incorrectly determines a merge should occur."
>

Are you done ranting? Get the @#$% bug patched.

--Mark.


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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Ben Myers]

On Tue, Sep 24, 2013 at 12:35:44PM -0500, Mark Tinguely wrote:
> On 09/23/13 18:48, Dave Chinner wrote:
> >On Mon, Sep 23, 2013 at 12:18:58PM -0500, Mark Tinguely wrote:
> >>Commit f5ea1100 cleans up the disk to host conversions for
> >>node directory entries, but because a variable is reused in
> >>xfs_node_toosmall() the next node is not correctly found.
> >>If the original node is small enough (<= 3/8 of the node size),
> >>this change may incorrectly cause a node collapse when it should
> >>not.
> >
> >The comment about the size of the node triggering a collapse is
> >irrelevant - nodes always collapse at that given size. What this
> >doesn't tell us is why the crash occurs - "the next node is not
> >correctly found" is not particularly obvious, and would require
> >quite a bit of code reading to work out from first principles a
> >couple of years down the track.
> >
> >The commit message should be more precise and describe what the
> >underlying cause of the failure was. i.e. that the node is finding itself as the merge
> >candidate because we go forward, overwrite the pointers and the new
> >block's backward sibling is the original block which is where we end
> >up on teh second loop.  And vice versa if we go backwards first...
> >
> >Also, the "next node" is correctly termed a "sibling", and it's
> >either the forwards or backwards sibling, not the "next" sibling as
> >the direction of movement is important. So perhaps this
> >is better written as:
> >
> >"When a node is considered for a merge with a sibling, it overwrites
> >the sibling pointers of the original node with the sibling's
> >pointers. This leads to loop considering the original node as a
> >merge candidate with itself in the second pass, and so it
> >incorrectly determines a merge should occur."
> >
> 
> Are you done ranting? Get the @#$% bug patched.

/me rolls his eyes.

I gather this means you're not inclined to repost.  I'll add Dave's suggestions
to the commit header and move on.

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

Applied.

-Ben

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Dave Chinner]

On Tue, Sep 24, 2013 at 12:35:44PM -0500, Mark Tinguely wrote:
> On 09/23/13 18:48, Dave Chinner wrote:
> >On Mon, Sep 23, 2013 at 12:18:58PM -0500, Mark Tinguely wrote:
> >>Commit f5ea1100 cleans up the disk to host conversions for
> >>node directory entries, but because a variable is reused in
> >>xfs_node_toosmall() the next node is not correctly found.
> >>If the original node is small enough (<= 3/8 of the node size),
> >>this change may incorrectly cause a node collapse when it should
> >>not.
> >
> >The comment about the size of the node triggering a collapse is
> >irrelevant - nodes always collapse at that given size. What this
> >doesn't tell us is why the crash occurs - "the next node is not
> >correctly found" is not particularly obvious, and would require
> >quite a bit of code reading to work out from first principles a
> >couple of years down the track.
> >
> >The commit message should be more precise and describe what the
> >underlying cause of the failure was. i.e. that the node is finding itself as the merge
> >candidate because we go forward, overwrite the pointers and the new
> >block's backward sibling is the original block which is where we end
> >up on teh second loop.  And vice versa if we go backwards first...
> >
> >Also, the "next node" is correctly termed a "sibling", and it's
> >either the forwards or backwards sibling, not the "next" sibling as
> >the direction of movement is important. So perhaps this
> >is better written as:
> >
> >"When a node is considered for a merge with a sibling, it overwrites
> >the sibling pointers of the original node with the sibling's
> >pointers. This leads to loop considering the original node as a
> >merge candidate with itself in the second pass, and so it
> >incorrectly determines a merge should occur."
> >
> 
> Are you done ranting? Get the @#$% bug patched.

I'm deeply sorry you feel that way about the review process - it's
not just code that matters. Experience has shown us time and time
again that accurate and complete commit messages are extremely
valuable as they document the symptoms of a problem being fixed and
why the fix was needed.

If someone needs to look at this commit in a couple of years to
determine if it matches a problem that a customer reported, they
shouldn't have to work out what the problem was and guess at it's
symptoms and impact from code analysis. The commit message should
tell them all the information they need(*).

It took you quite some time and effort to find the problem, so it's
worthy of spending a few minutes to document that effort for
posterity. That way when someone asks you a question about the
problem, all you need to do is point them at the commit and all the
information is right at their fingertips.

I'm not asking you to do anything I don't do already.  Have you ever
wondered why I write long, verbose commit messages and changes with
verbose comments? They aren't for the reviewer - I can answer
questions in real-time about the change. The message is for someone
looking at the change in 2-3 years time when when nobody remembers
the exact circumstances of the fix anymore.

IOWs, by clearly documenting the problem being fixed, the root cause
analysis and verification that was performed *using standard
terminology*, we make it far easier for someone to come along in 2-3
years time and understand the fix without needing any further
information about it.

Software engineering best practices have come a long way since the
early 90's - writing meaningful commit messages to go along with
your code changes has been considered a best practise for at least
the last 10 years....

Cheers,

Dave.

(*) If you've ever spent any time looking at the old XFS archives,
then you'll understand exactly why what I've asked for is important.
Trying to reverse-engineer why a change was made in the old XFS code
is just about impossible because all they generally have is single
line commit messages and nothing else to describe the change.
Sometimes they just point at a bug number, without any other
information at all.....
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Mark Tinguely]

On 09/24/13 16:06, Dave Chinner wrote:
> On Tue, Sep 24, 2013 at 12:35:44PM -0500, Mark Tinguely wrote:
>> On 09/23/13 18:48, Dave Chinner wrote:
>>> On Mon, Sep 23, 2013 at 12:18:58PM -0500, Mark Tinguely wrote:
>>>> Commit f5ea1100 cleans up the disk to host conversions for
>>>> node directory entries, but because a variable is reused in
>>>> xfs_node_toosmall() the next node is not correctly found.
>>>> If the original node is small enough (<= 3/8 of the node size),
>>>> this change may incorrectly cause a node collapse when it should
>>>> not.
>>>
>>> The comment about the size of the node triggering a collapse is
>>> irrelevant - nodes always collapse at that given size. What this
>>> doesn't tell us is why the crash occurs - "the next node is not
>>> correctly found" is not particularly obvious, and would require
>>> quite a bit of code reading to work out from first principles a
>>> couple of years down the track.
>>>
>>> The commit message should be more precise and describe what the
>>> underlying cause of the failure was. i.e. that the node is finding itself as the merge
>>> candidate because we go forward, overwrite the pointers and the new
>>> block's backward sibling is the original block which is where we end
>>> up on teh second loop.  And vice versa if we go backwards first...
>>>
>>> Also, the "next node" is correctly termed a "sibling", and it's
>>> either the forwards or backwards sibling, not the "next" sibling as
>>> the direction of movement is important. So perhaps this
>>> is better written as:
>>>
>>> "When a node is considered for a merge with a sibling, it overwrites
>>> the sibling pointers of the original node with the sibling's
>>> pointers. This leads to loop considering the original node as a
>>> merge candidate with itself in the second pass, and so it
>>> incorrectly determines a merge should occur."
>>>
>>
>> Are you done ranting? Get the @#$% bug patched.
>
> I'm deeply sorry you feel that way about the review process - it's
> not just code that matters. Experience has shown us time and time
> again that accurate and complete commit messages are extremely
> valuable as they document the symptoms of a problem being fixed and
> why the fix was needed.
>
> If someone needs to look at this commit in a couple of years to
> determine if it matches a problem that a customer reported, they
> shouldn't have to work out what the problem was and guess at it's
> symptoms and impact from code analysis. The commit message should
> tell them all the information they need(*).
>
> It took you quite some time and effort to find the problem, so it's
> worthy of spending a few minutes to document that effort for
> posterity. That way when someone asks you a question about the
> problem, all you need to do is point them at the commit and all the
> information is right at their fingertips.
>
> I'm not asking you to do anything I don't do already.  Have you ever
> wondered why I write long, verbose commit messages and changes with
> verbose comments? They aren't for the reviewer - I can answer
> questions in real-time about the change. The message is for someone
> looking at the change in 2-3 years time when when nobody remembers
> the exact circumstances of the fix anymore.
>
> IOWs, by clearly documenting the problem being fixed, the root cause
> analysis and verification that was performed *using standard
> terminology*, we make it far easier for someone to come along in 2-3
> years time and understand the fix without needing any further
> information about it.
>
> Software engineering best practices have come a long way since the
> early 90's - writing meaningful commit messages to go along with
> your code changes has been considered a best practise for at least
> the last 10 years....
>
> Cheers,
>
> Dave.
>
> (*) If you've ever spent any time looking at the old XFS archives,
> then you'll understand exactly why what I've asked for is important.
> Trying to reverse-engineer why a change was made in the old XFS code
> is just about impossible because all they generally have is single
> line commit messages and nothing else to describe the change.
> Sometimes they just point at a bug number, without any other
> information at all.....

You do not always practice what you preach - here is the commit message 
from the patch that caused this whole problem:

commit f5ea110044fa858925a880b4fa9f551bfa2dfc38
Author: Dave Chinner <dchinner@redhat.com>
Date:   Wed Apr 24 18:58:02 2013 +1000

     xfs: add CRCs to dir2/da node blocks

     Signed-off-by: Dave Chinner <dchinner@redhat.com>
     Reviewed-by: Ben Myers <bpm@sgi.com>
     Signed-off-by: Ben Myers <bpm@sgi.com>

Did this patch just add CRCs to node blocks? no.

And how about your patches that do not need a commit message at all?!
Are these exceptions? No, I have done enough reviews of your patches, 
and some of your commit message are less than stunning.

when your preaching, and not so subtle digs at me or SGI gets to the 
point of rewriting commit messages even I can't hold my tongue any more.

PS. I told Ben that I do not appreciate commit messages being changed.

--Mark.

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Dave Chinner]

On Tue, Sep 24, 2013 at 04:34:53PM -0500, Mark Tinguely wrote:
> On 09/24/13 16:06, Dave Chinner wrote:
> >On Tue, Sep 24, 2013 at 12:35:44PM -0500, Mark Tinguely wrote:
> >>On 09/23/13 18:48, Dave Chinner wrote:
> >>>On Mon, Sep 23, 2013 at 12:18:58PM -0500, Mark Tinguely wrote:
> >>>>Commit f5ea1100 cleans up the disk to host conversions for
> >>>>node directory entries, but because a variable is reused in
> >>>>xfs_node_toosmall() the next node is not correctly found.
> >>>>If the original node is small enough (<= 3/8 of the node size),
> >>>>this change may incorrectly cause a node collapse when it should
> >>>>not.
....
> >>>Also, the "next node" is correctly termed a "sibling", and it's
> >>>either the forwards or backwards sibling, not the "next" sibling as
> >>>the direction of movement is important. So perhaps this
> >>>is better written as:
> >>>
> >>>"When a node is considered for a merge with a sibling, it overwrites
> >>>the sibling pointers of the original node with the sibling's
> >>>pointers. This leads to loop considering the original node as a
> >>>merge candidate with itself in the second pass, and so it
> >>>incorrectly determines a merge should occur."
> >>>
> >>
> >>Are you done ranting? Get the @#$% bug patched.
> >
> >I'm deeply sorry you feel that way about the review process - it's
> >not just code that matters. Experience has shown us time and time
> >again that accurate and complete commit messages are extremely
> >valuable as they document the symptoms of a problem being fixed and
> >why the fix was needed.
.....
> 
> You do not always practice what you preach - here is the commit
> message from the patch that caused this whole problem:
> 
> commit f5ea110044fa858925a880b4fa9f551bfa2dfc38
> Author: Dave Chinner <dchinner@redhat.com>
> Date:   Wed Apr 24 18:58:02 2013 +1000
> 
>     xfs: add CRCs to dir2/da node blocks
> 
>     Signed-off-by: Dave Chinner <dchinner@redhat.com>
>     Reviewed-by: Ben Myers <bpm@sgi.com>
>     Signed-off-by: Ben Myers <bpm@sgi.com>
> 
> Did this patch just add CRCs to node blocks? no.
> 
> And how about your patches that do not need a commit message at all?!

The above commit has a very different context surrounding it - it
was in the middle of a major feaure addition series where the
process of additions had been described in the previous commits, and
the one-line description is sufficient for future readers to
determine that the commit is not a bug fix.

If a reviewer doesn't speak up about something, then that implies
the reviewer considers it acceptible. In this case, the reviewer
(Ben) seemed to think the one-line description of the commit was
acceptible for the context in which it was being proposed and
committed.

> Are these exceptions? No, I have done enough reviews of your
> patches, and some of your commit message are less than stunning.

Sure, some of my commit messages need work, especially as I'm
spelling and grammar challenged at the best of times. I make no
claim that they are perfect, but I do try to make the sufficient for
the context in which they are being proposed.

However - it is *your responsibility as a reviewer* to ask the
author to write a more complete change log or fix omissions or
clarify parts of the commit message that need improvement. If the
reviewer does not ask for improvements or you chose not to review
the proposed patches, then you have no grounds to complain about the
contents of patches that were committed on your watch...

> when your preaching, and not so subtle digs at me or SGI gets to the
> point of rewriting commit messages even I can't hold my tongue any
> more.

You are reading way too much into a simple request, Mark.
Constructive criticism of your code is not a personal attack.

Quite frankly, I don't care who submits the code or who they work
for - I apply the same *peer review criteria* to all code that is
proposed.  I'm here to make sure the code is robust, solid and
maintainable into the forseeable future. I'm here to tear your
patches apart and then help put it back together better than it was
before. It simply doesn't matter who submits the code...

> PS. I told Ben that I do not appreciate commit messages being changed.

Ben did the right thing. Maintainers rewrite commit messages to
improve them all the time...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Ben Myers]

Hey Dave,

On Wed, Sep 25, 2013 at 09:33:31AM +1000, Dave Chinner wrote:
> On Tue, Sep 24, 2013 at 04:34:53PM -0500, Mark Tinguely wrote:
> > On 09/24/13 16:06, Dave Chinner wrote:
> > >On Tue, Sep 24, 2013 at 12:35:44PM -0500, Mark Tinguely wrote:
> > >>On 09/23/13 18:48, Dave Chinner wrote:
> > >>>On Mon, Sep 23, 2013 at 12:18:58PM -0500, Mark Tinguely wrote:
> > >>>>Commit f5ea1100 cleans up the disk to host conversions for
> > >>>>node directory entries, but because a variable is reused in
> > >>>>xfs_node_toosmall() the next node is not correctly found.
> > >>>>If the original node is small enough (<= 3/8 of the node size),
> > >>>>this change may incorrectly cause a node collapse when it should
> > >>>>not.
> ....
> > >>>Also, the "next node" is correctly termed a "sibling", and it's
> > >>>either the forwards or backwards sibling, not the "next" sibling as
> > >>>the direction of movement is important. So perhaps this
> > >>>is better written as:
> > >>>
> > >>>"When a node is considered for a merge with a sibling, it overwrites
> > >>>the sibling pointers of the original node with the sibling's
> > >>>pointers. This leads to loop considering the original node as a
> > >>>merge candidate with itself in the second pass, and so it
> > >>>incorrectly determines a merge should occur."
> > >>>
> > >>
> > >>Are you done ranting? Get the @#$% bug patched.
> > >
> > >I'm deeply sorry you feel that way about the review process - it's
> > >not just code that matters. Experience has shown us time and time
> > >again that accurate and complete commit messages are extremely
> > >valuable as they document the symptoms of a problem being fixed and
> > >why the fix was needed.
> .....
> > 
> > You do not always practice what you preach - here is the commit
> > message from the patch that caused this whole problem:
> > 
> > commit f5ea110044fa858925a880b4fa9f551bfa2dfc38
> > Author: Dave Chinner <dchinner@redhat.com>
> > Date:   Wed Apr 24 18:58:02 2013 +1000
> > 
> >     xfs: add CRCs to dir2/da node blocks
> > 
> >     Signed-off-by: Dave Chinner <dchinner@redhat.com>
> >     Reviewed-by: Ben Myers <bpm@sgi.com>
> >     Signed-off-by: Ben Myers <bpm@sgi.com>
> > 
> > Did this patch just add CRCs to node blocks? no.
> > 
> > And how about your patches that do not need a commit message at all?!
> 
> The above commit has a very different context surrounding it - it
> was in the middle of a major feaure addition series where the
> process of additions had been described in the previous commits, and
> the one-line description is sufficient for future readers to
> determine that the commit is not a bug fix.
> 
> If a reviewer doesn't speak up about something, then that implies
> the reviewer considers it acceptible.

I disagree with that statement.  A reviewer might not speak up about a
flaw in a work for for other reasons.  There are more than two
alternatives in this context.  For example...

> In this case, the reviewer
> (Ben) seemed to think the one-line description of the commit was
> acceptible for the context in which it was being proposed and
> committed.

I wasn't happy with the lack of the description in those commits, and
probably should have spoken up about it.  In this case I weighed the
frustration due to the argument that would almost certainly have ensued
against the benefit of a decent commit message, and punted.

That's similar to what happened when I pulled in the 1st half of the
userspace crc-dev work after you refused to address my concerns in
review.  I punted again, because the argument would have been more
nuisance than it was worth.

Clearly I need to stop punting so often.  But since many a suggestion
made by Mark or myself is taken as a bloody travesty and an argument
ensues... I have to admit to some fatigue in this area.

> > Are these exceptions? No, I have done enough reviews of your
> > patches, and some of your commit message are less than stunning.
> 
> Sure, some of my commit messages need work, especially as I'm
> spelling and grammar challenged at the best of times. I make no
> claim that they are perfect, but I do try to make the sufficient for
> the context in which they are being proposed.
> 
> However - it is *your responsibility as a reviewer* to ask the
> author to write a more complete change log or fix omissions or
> clarify parts of the commit message that need improvement. 

...

> If the
> reviewer does not ask for improvements or you chose not to review
> the proposed patches, then you have no grounds to complain about the
> contents of patches that were committed on your watch...

I disagree.  The act of committing a patch does not necessarily imply an
agreement regarding it's contents.  I am often in a position where I
have to commit patches that I don't fully agree with.  This doesn't
imply that I've waived my right to whine about bad commits later.  ;)

> > when your preaching, and not so subtle digs at me or SGI gets to the
> > point of rewriting commit messages even I can't hold my tongue any
> > more.
> 
> You are reading way too much into a simple request, Mark.
> Constructive criticism of your code is not a personal attack.

I suspect the issue is not really with this particular request.  There
is a cumulative effect to the toxicity that you seem to feel the need to
bring to the list, and it sounds like Mark is full up.

This is gonna sound soft coming from a grown man, but I'll call it for
what I think it is:  Bullying.  And to the extent that I've taken part
or watched others get planed and done nothing, I think I have to take
this as a stain on my own character.

The constructive criticism is fine.  The bullying needs to stop.
Technical prowess is not license to act badly.  Knock it off.
 
> Quite frankly, I don't care who submits the code or who they work for
> - I apply the same *peer review criteria* to all code that is
> proposed.  I'm here to make sure the code is robust, solid and
> maintainable into the forseeable future. I'm here to tear your patches
> apart and then help put it back together better than it was before. It
> simply doesn't matter who submits the code...

I'm pleased to hear that you don't care who submits code, but it sure
seems like you're grinding an axe for Mark to me.

> > PS. I told Ben that I do not appreciate commit messages being
> > changed.
> 
> Ben did the right thing. Maintainers rewrite commit messages to
> improve them all the time...

I handled it badly.  It was probably ok to pull in your suggestion into
the commit message as long as it was attributed properly, but it was
more aggressive than was necessary, and I can understand why Mark didn't
appreciate it.  Also, I really needn't have been snarky about it.
Apologies to Mark.

Regards,
Ben

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Eric Sandeen]

[stable@vger.kernel.org stripped from this fascinating thread]

On 9/25/13 1:38 PM, Ben Myers wrote:
> Hey Dave,
> 
> On Wed, Sep 25, 2013 at 09:33:31AM +1000, Dave Chinner wrote:

<snip>

>> If a reviewer doesn't speak up about something, then that implies
>> the reviewer considers it acceptible.
> 
> I disagree with that statement.  A reviewer might not speak up about a
> flaw in a work for for other reasons.  There are more than two
> alternatives in this context.  For example...

<snip>

<apologies if I snipped too much context, not my intention>

>> If the
>> reviewer does not ask for improvements or you chose not to review
>> the proposed patches, then you have no grounds to complain about the
>> contents of patches that were committed on your watch...
> 
> I disagree.  The act of committing a patch does not necessarily imply an
> agreement regarding it's contents.  I am often in a position where I
> have to commit patches that I don't fully agree with.  This doesn't
> imply that I've waived my right to whine about bad commits later.  ;)

If you don't like it, don't add reviewed-by.

If you don't like the reviews, don't sign off, don't merge it.
As maintainer you have that right, but to be a good maintainer,
you need a strong reason.  And if you have a strong technical concern,
then it's the patch author's duty to take it seriously, or risk not
getting the patch merged.  And the author might argue back.  And other
people might argue back.  And in the end, if we can all keep our cool,
the code will be better for it.  If you apply clear and consistent merge
criteria then people will know what to expect.

When you send a Reviewed-by: that's a pretty strong indication of agreement.
There shouldn't be a lot of misunderstanding about what it means;
the kernel doc (Documentation/SubmittingPatches) is very clear:

===
        Reviewer's statement of oversight

        By offering my Reviewed-by: tag, I state that:

         (a) I have carried out a technical review of this patch to
             evaluate its appropriateness and readiness for inclusion into
             the mainline kernel.

         (b) Any problems, concerns, or questions relating to the patch
             have been communicated back to the submitter.  I am satisfied
             with the submitter's response to my comments.

         (c) While there may be things that could be improved with this
             submission, I believe that it is, at this time, (1) a
             worthwhile modification to the kernel, and (2) free of known
             issues which would argue against its inclusion.

         (d) While I have reviewed the patch and believe it to be sound, I
             do not (unless explicitly stated elsewhere) make any
             warranties or guarantees that it will achieve its stated
             purpose or function properly in any given situation.
===

If you can't get behind that, don't add your Reviewed-by: and continue to
work it out until you can.  Reviewers should be sure to pay attention to
point (c) as well, something I sometimes forget myself.

Reviews don't have to be heeded, either.  They don't have to be gauntlets
thrown down or lines in the sand.  "You've corrupted memory here" is
different from "you could use an args struct instead of 8 arguments"
or "your commit log isn't very descriptive."  Discretion abounds.

As a reviewer once said on another list, "I'm free to share
what occurs to me and you're free to tell me to go jump in a lake."
Especially for cosmetic issues, that's a pretty good POV.  Sometimes
it's trickier.

As maintainer I guess you get to decide if a review concern has enough
merit to hold up merging.  Ignoring a compelling technical concern
could be risky.

Maybe one reviewer is dissatisfied, and another is satisfied.  Then
you get to play Solomon again.  Life goes on, I hope.

-Eric

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

Re: [PATCH] xfs: v2 fix node forward in xfs_node_toosmall

[Ben Myers]

Hey,

On Wed, Sep 25, 2013 at 04:03:06PM -0500, Eric Sandeen wrote:
> [stable@vger.kernel.org stripped from this fascinating thread]

Good idea.  I should have done, in retrospect.

> On 9/25/13 1:38 PM, Ben Myers wrote:
> > Hey Dave,
> > 
> > On Wed, Sep 25, 2013 at 09:33:31AM +1000, Dave Chinner wrote:
> 
> <snip>
> 
> >> If a reviewer doesn't speak up about something, then that implies
> >> the reviewer considers it acceptible.
> > 
> > I disagree with that statement.  A reviewer might not speak up about a
> > flaw in a work for for other reasons.  There are more than two
> > alternatives in this context.  For example...
> 
> <snip>
> 
> <apologies if I snipped too much context, not my intention>
> 
> >> If the
> >> reviewer does not ask for improvements or you chose not to review
> >> the proposed patches, then you have no grounds to complain about the
> >> contents of patches that were committed on your watch...
> > 
> > I disagree.  The act of committing a patch does not necessarily imply an
> > agreement regarding it's contents.  I am often in a position where I
> > have to commit patches that I don't fully agree with.  This doesn't
> > imply that I've waived my right to whine about bad commits later.  ;)
> 
> If you don't like it, don't add reviewed-by.
> 
> If you don't like the reviews, don't sign off, don't merge it.
> As maintainer you have that right, but to be a good maintainer,
> you need a strong reason.  And if you have a strong technical concern,
> then it's the patch author's duty to take it seriously, or risk not
> getting the patch merged.  And the author might argue back.  And other
> people might argue back.  And in the end, if we can all keep our cool,
> the code will be better for it.  If you apply clear and consistent merge
> criteria then people will know what to expect.
> 
> When you send a Reviewed-by: that's a pretty strong indication of agreement.

Gah.  Yeah, I overstated.  Maybe 'does not necessarily imply an agreement'
should read something more like 'does not necessarily imply it is beyond
criticism', that's a bit more reasonable.

> There shouldn't be a lot of misunderstanding about what it means;
> the kernel doc (Documentation/SubmittingPatches) is very clear:
> 
> ===
>         Reviewer's statement of oversight
> 
>         By offering my Reviewed-by: tag, I state that:
> 
>          (a) I have carried out a technical review of this patch to
>              evaluate its appropriateness and readiness for inclusion into
>              the mainline kernel.
> 
>          (b) Any problems, concerns, or questions relating to the patch
>              have been communicated back to the submitter.  I am satisfied
>              with the submitter's response to my comments.
> 
>          (c) While there may be things that could be improved with this
>              submission, I believe that it is, at this time, (1) a
>              worthwhile modification to the kernel, and (2) free of known
>              issues which would argue against its inclusion.
> 
>          (d) While I have reviewed the patch and believe it to be sound, I
>              do not (unless explicitly stated elsewhere) make any
>              warranties or guarantees that it will achieve its stated
>              purpose or function properly in any given situation.
> ===
> 
> If you can't get behind that, don't add your Reviewed-by: and continue to
> work it out until you can.  Reviewers should be sure to pay attention to
> point (c) as well, something I sometimes forget myself.

I haven't read that in awhile.  Nice to have a brush up, thanks.  I think it
probably comes short of '...you have no grounds to complain about the contents
of patches that were committed on your watch...', which is the idea I disagree
with.  Maybe others don't think so, you could argue that I failed on point b in
my review by not choosing to hound Dave for a commit message.  ;)

> Reviews don't have to be heeded, either.  They don't have to be gauntlets
> thrown down or lines in the sand.  "You've corrupted memory here" is
> different from "you could use an args struct instead of 8 arguments"
> or "your commit log isn't very descriptive."  Discretion abounds.
>
> As a reviewer once said on another list, "I'm free to share
> what occurs to me and you're free to tell me to go jump in a lake."
> Especially for cosmetic issues, that's a pretty good POV.  Sometimes
> it's trickier.
> 
> As maintainer I guess you get to decide if a review concern has enough
> merit to hold up merging.  Ignoring a compelling technical concern
> could be risky.

I agree that ignoring a compelling technical concern can be risky.

> Maybe one reviewer is dissatisfied, and another is satisfied.  Then
> you get to play Solomon again.  Life goes on, I hope.

Yeah.  Life goes on.

Thanks Eric,
	Ben

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