On Thu, Feb 24, 2011 at 7:35 AM, Andreas Dilger <adilger@xxxxxxxxx> wrote:
> On 2011-02-23, at 8:59 AM, Yongqiang Yang wrote:
>> @@ -3788,17 +3788,27 @@ static int ext4_ext_fiemap_cb(struct inode *inode,
>> - if (newex->ec_start == 0) {
>> + if (!newex->ec_start) {
>
> (style) the original code is actually correct. ec_start is not a boolean, so comparing it == 0 is actually the right thing to do.
>
>> @@ -3807,8 +3817,13 @@ static int ext4_ext_fiemap_cb(struct inode *inode, struct ext4_ext_path *path,
>> if (!bh)
>> return EXT_CONTINUE;
>>
>> + /* Assume block-size equals page-size. */
>
> This is not a valid assumption.
>
>> if (buffer_delay(bh)) {
>> flags |= FIEMAP_EXTENT_DELALLOC;
>> + if (page->index > offset) {
>> + logical = ((__u64)page->index << PAGE_SHIFT);
>> + newex->ec_block = logical >> blksize_bits;
>> + }
>
> So, this assumes that the entire unmapped extent is described by the first page, but doesn't actually check whether all of the pages exist. For the purpose of cp it might be OK, since at worst it means that cp will be reading from a hole in the source file. However, I wonder if other applications will depend on the allocated extent being more accurate?
>
>> +EXPORT_SYMBOL(find_get_pages);
>
> Eric had also suggested pagevec_lookup_tag(), which is already exported.
Yes, we can use this function. I will make a new patch.
>
> Cheers, Andreas
>
>
>
>
>
>
--
Best Wishes
Yongqiang Yang
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
