xfs
[Top] [All Lists]

Re: [PATCH] xfs: improve xfs_iext_destroy() by freeing extent indirectio

To: Dave Chinner <david@xxxxxxxxxxxxx>
Subject: Re: [PATCH] xfs: improve xfs_iext_destroy() by freeing extent indirection array directly
From: Jeff Liu <jeff.liu@xxxxxxxxxx>
Date: Mon, 23 Sep 2013 12:56:56 +0800
Cc: "xfs@xxxxxxxxxxx" <xfs@xxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20130923003617.GM12541@dastard>
References: <523C5E92.8000406@xxxxxxxxxx> <20130923003617.GM12541@dastard>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:11.0) Gecko/20120410 Thunderbird/11.0.1
On 09/23/2013 08:36 AM, Dave Chinner wrote:

> On Fri, Sep 20, 2013 at 10:41:22PM +0800, Jeff Liu wrote:
>> From: Jie Liu <jeff.liu@xxxxxxxxxx>
>>
>> To free the incore file extents stores at the indirection array, we
>> call the common routine xfs_iext_irec_remove() to remove a record
>> from the array one at a time in reverse order, which will resize an
>> extent indirection array repeatedly according to the array size.
>>
>> This is not often the case to make a file with thousands extent records
>> stores at an indirection array, but above operation is inefficient and
>> could result in memory fragments.
> 
> Yes, it may be inefficient, but I don't see that it's a contributor
> to memory fragmentation as the reallocated buffer is freed shortly
> after it has been allocated as the array shrinks. Do you have any
> evidence to suggest that such behaviour is actually fragmenting
> memory? If so, is the any test case that reproduces this problem?

Ah, yes, it should not cause memory fragmentation.

The benefits is that this change could save alloc/free buffers depending
on the number of extents records are stored at indirection array.

> 
> How did you test the change?

I only test this change with a simple case for creating a sparse file
with 8192 extents, which was shown as following,

xfs_io -f -c "truncate 10g" /xfs/testme

for i in $(seq 0 1 8191); do
        offset=$(($i * $((1 << 20))))
        xfs_io -c "pwrite $offset 1k" /xfs/testme
done

> 
>> This patch refine xfs_iext_destroy() by freeing the extent records from
>> the indirection array directly in this case.
>>
>> Signed-off-by: Jie Liu <jeff.liu@xxxxxxxxxx>
>> ---
> 
> FWIW, it is best to title a resend as [PATCH x/y, V2], and here tell
> us what changed between posts such as:
> 
> V2:
> - fixed typo in original posting

Ok. :)

Thanks,
-Jeff

> 
>>  fs/xfs/xfs_inode_fork.c |    7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_inode_fork.c b/fs/xfs/xfs_inode_fork.c
>> index 02f1083..ba70f98 100644
>> --- a/fs/xfs/xfs_inode_fork.c
>> +++ b/fs/xfs/xfs_inode_fork.c
>> @@ -1525,9 +1525,12 @@ xfs_iext_destroy(
>>              int     nlists;
>>  
>>              nlists = ifp->if_real_bytes / XFS_IEXT_BUFSZ;
>> -            for (erp_idx = nlists - 1; erp_idx >= 0 ; erp_idx--) {
>> -                    xfs_iext_irec_remove(ifp, erp_idx);
>> +            for (erp_idx = 0; erp_idx < nlists; erp_idx++) {
>> +                    xfs_ext_irec_t  *erp = &ifp->if_u1.if_ext_irec[erp_idx];
>> +                    if (erp->er_extbuf)
>> +                            kmem_free(erp->er_extbuf);
>>              }
>> +            kmem_free(ifp->if_u1.if_ext_irec);
> 
> 
> The code looks correct...
> 
> Cheers,
> 
> Dave.


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