xfs
[Top] [All Lists]

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

To: "Christoph Hellwig" <hch@xxxxxxxxxxxxx>, "Dave Chinner" <david@xxxxxxxxxxxxx>
Subject: RE: [PATCH] xfs: xfs_swap_extents needs to handle dynamic fork offsets
From: "Alex Elder" <aelder@xxxxxxx>
Date: Wed, 13 Jan 2010 16:32:12 -0600
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20100112130922.GA30985@xxxxxxxxxxxxx>
Thread-index: AcqTi2yPc65tn26dRb60/pK2KygrTABE9z+w
Thread-topic: [PATCH] xfs: xfs_swap_extents needs to handle dynamic fork offsets
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

<Prev in Thread] Current Thread [Next in Thread>