Tao Ma wrote:
quoted text
> Eric Sandeen wrote:
>> Tao Ma wrote:
>>  
>>> In xfs_vn_fiemap, we set bvm_count to fi_extent_max + 1 and want
>>> to return fi_extent_max extents, but actually it won't work for
>>> a sparse file. The reason is that in xfs_getbmap we will
>>> calculate holes and set it in 'out', while out is malloced by
>>> bmv_count(fi_extent_max+1) which didn't consider holes. So in the
>>> worst case, if 'out' vector looks like
>>> [hole, extent, hole, extent, hole, ... hole, extent, hole],
>>> we will only return half of fi_extent_max extents.
>>>
>>> So in xfs_vn_fiemap, we should consider this worst case. If the
>>> user wants fi_extent_max extents, we need a 'out' with size of
>>> 2 *fi_extent_max + 1.
>>>     
>>
>> This all seems right to me, though your commit message above (+1)
>> doesn't match the comment and code in the patch (+2)
>>   
> oh, yes, I will change the commit log and send a v2.
> and can I add your ack for this patch?

Sure, it seems right to me.

Thanks,
-Eric

quoted text
> Regards,
> Tao
>> -Eric
>>
>>  
>>> Cc: Alex Elder <aelder@sgi.com>
>>> Cc: Christoph Hellwig <hch@lst.de>
>>> Cc: Dave Chinner <david@fromorbit.com>
>>> Signed-off-by: Tao Ma <tao.ma@oracle.com>
>>> ---
>>>  fs/xfs/linux-2.6/xfs_iops.c |   16 ++++++++++++++--
>>>  1 files changed, 14 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/xfs/linux-2.6/xfs_iops.c b/fs/xfs/linux-2.6/xfs_iops.c
>>> index 9c8019c..1db92e3 100644
>>> --- a/fs/xfs/linux-2.6/xfs_iops.c
>>> +++ b/fs/xfs/linux-2.6/xfs_iops.c
>>> @@ -672,9 +672,21 @@ xfs_vn_fiemap(
>>>      else
>>>          bm.bmv_length = BTOBB(length);
>>>  
>>> -    /* We add one because in getbmap world count includes the header */
>>> +    /*
>>> +     * It is a bit tricky for us to calculate the bmv_count from
>>> +     * fi_extent_max.
>>> +     * If we support to return fi_extent_max extents to the user,
>>> +     * we need at most 2 * fi_extent_max + 1 for bmv_count since
>>> +     * in xfs_getbmap we will calculate holes while fi_extent_max
>>> +     * don't have them. So in the worst case, bmv can looks like
>>> +     * [hole, extent, hole, extent, hole, ... hole, extent, hole].
>>> +     * So there will be 2 *fi_extent_max + 1.
>>> +     * What's more, in getbmap world count have to include the
>>> +     * header, so we need another bmv. So the total number will
>>> +     * be 2 * fieinfo->fi_extents_max + 2.
>>> +     */
>>>      bm.bmv_count = !fieinfo->fi_extents_max ? MAXEXTNUM :
>>> -                    fieinfo->fi_extents_max + 1;
>>> +            2 * fieinfo->fi_extents_max + 2;
>>>      bm.bmv_count = min_t(__s32, bm.bmv_count,
>>>                   (PAGE_SIZE * 16 / sizeof(struct getbmapx)));
>>>      bm.bmv_iflags = BMV_IF_PREALLOC;
>>>     
>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe
>> linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>   
> 

--
unsubscribe notice
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/