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

[Mark Tinguely <tinguely@sgi.com>]

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