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.
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
