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@xxxxxxx> Applied. -Ben -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html