[PATCH] xfs: xfs_swap_extents needs to handle dynamic fork offsets

Alex Elder aelder at sgi.com
Wed Jan 13 16:32:12 CST 2010


Christoph Hellwig wrote:
>> +	TP_printk("dev %d:%d %s inode 0x%llx, %s format, num_extents %d, "
>> +		  "Max in-fork extents %d, broot size %d, fork offset %d",
> 
> It would be nice to keep the
> 
> 	"dev %d:%d ino 0x%llx"
> 
> prefix as a convention so that all trace records are similar at their
> beginning.

Perhaps:
    +	TP_printk("dev %d:%d inode 0x%llx (%s), %s format, num_extents %d, " 
                                          ^
                                          +--- symbolic entry->which

>>  #undef TRACE_INCLUDE_PATH
>> diff --git a/fs/xfs/xfs_dfrag.c b/fs/xfs/xfs_dfrag.c
>> @@ -53,7 +53,7 @@ xfs_swapext(
>>  	xfs_swapext_t	*sxp)
>>  {
>>  	xfs_inode_t     *ip, *tip;
>> -	struct file	*file, *target_file;
>> +	struct file	*file, *tmp_file;
> 
> I think these xfs_swapext belong into a separate patch.  While they
> make the code quite a bit more redable they're purely cleanups and
> can wait for 2.6.34.  And while you're at it you might also want to
> merge xfs_swap_extents into xfs_swapext - there's no need for that
> split at all.

I agree.  The change here is good (it was confusing and wrong before).
But Dave can you please re-submit this with only the critical changes
so I can get them to Linus in this release cycle?  I think the trace
addition is probably fine, just get rid of the gratuitous variable
name change in xfs_swapext() and put that in a separate patch.

The rest looks all good.  Thanks.
	
					-Alex




More information about the xfs mailing list