>On Wed, Jul 31, 2013 at 9:36 AM, majianpeng <majianpeng@xxxxxxxxx> wrote:
>> [snip]
>> I think this patch can do work:
>> Those case which i tested
>> A: filesize=0, buffer=1M
>> B: data[2M] | hole| data[2M], bs= 6M/7M
>
>I don't think your zero buffer change is correct for this test case.
>
dd if=/dev/urandom of=file bs=1M count=2 oflag=direct
dd if=/dev/urandom of=file bs=1M count=2 seek=4 oflag=direct
ls file
-rw-r--r-- 1 root root 6291456 Jul 31 12:46 file
debug message:
[78370.408220] ceph: file.c:355 : striped_read 0~7340032 (read 0) got 2097152 HITSTRIPE SHORT
[78370.409179] ceph: file.c:355 : striped_read 2097152~5242880 (read 2097152) got 0 HITSTRIPE HITHOLE
[78370.409182] ceph: file.c:362 : zero hole 2097152 to 4194304
[78370.431289] ceph: file.c:355 : striped_read 4194304~3145728 (read 4194304) got 2097152 SHORT
[78370.431294] ceph: file.c:399 : striped_read returns 6291456
Am i missing something?
>> C: data[4m] | hole | hole |data[2M] bs=16M/18M
>>
>> Are there some case ignore?
>> Thanks!
>> Jianpeng Ma
>>
>>
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index 2ddf061..96ce893 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -319,7 +319,7 @@ static int striped_read(struct inode *inode,
>> int read;
>> struct page **page_pos;
>> int ret;
>> - bool hit_stripe, was_short;
>> + bool hit_stripe, was_short, hit_hole = false;
>>
>> /*
>> * we may need to do multiple reads. not atomic, unfortunately.
>> @@ -342,21 +342,30 @@ more:
>> ci->i_truncate_seq,
>> ci->i_truncate_size,
>> page_pos, pages_left, page_align);
>> - if (ret == -ENOENT)
>> +
>> + if ((ret == 0 || ret == -ENOENT) && pos < inode->i_size)
>> + hit_hole = true;
>
>why do we need 'hit_hole', it's essential the same as was_short. The code
>is already so messy.
>I don't think adding 'hit_hole' make it more readable.
>
hit_hole means return value are ENOENT or zero.
But the short-read contain EOF beside above two case.
>> + else if (ret == -ENOENT)
>> ret = 0;
>> +
>> hit_stripe = this_len < left;
>> - was_short = ret >= 0 && ret < this_len;
>> - dout("striped_read %llu~%u (read %u) got %d%s%s\n", pos, left,
>read,
>> - ret, hit_stripe ? " HITSTRIPE" : "", was_short ? " SHORT" :
>"");
>> -
>> - if (ret > 0) {
>> - int didpages = (page_align + ret) >> PAGE_CACHE_SHIFT;
>> -
>> - if (read < pos - off) {
>> - dout(" zero gap %llu to %llu\n", off + read, pos);
>> - ceph_zero_page_vector_range(page_align + read,
>> - pos - off - read,
>pages);
>> + was_short = ret > 0 && ret < this_len;
>> + dout("striped_read %llu~%u (read %u) got %d%s%s%s\n", pos, left,
>read,
>> + ret, hit_stripe ? " HITSTRIPE" : "", was_short ? " SHORT" :
>"",
>> + hit_hole ? " HITHOLE" : "");
>> +
>> + if (ret > 0 || hit_hole) {
>> + int didpages;
>> +
>> + if (hit_hole) {
>> + ret = this_len;
>> + dout(" zero hole %llu to %llu\n", pos , pos +
>ret);
>> + ceph_zero_page_vector_range(page_align + read,
>> + ret, pages);
>> + hit_hole = false;
>this is incorrect for the 'ret > 0' and 'ret = -ENOENT' cases.
>
For the ret > 0, it can't do those.
For the ret = -ENOENT and pos + this_len > i_size mean , can you give me a example ?
Jianpeng Ma
>Besides, if 'pos + this_len >= i_size'. we should do nothing here. let the
>'zero trailing bytes'
>code do the job.
>
>Yan, Zheng
>
>> }
>> + didpages = (page_align + ret) >> PAGE_CACHE_SHIFT;
>> +
>> pos += ret;
>> read = pos - off;
>> left -= ret;
>
Thanks!
Jianpeng Ma
>On Wed, Jul 31, 2013 at 9:36 AM, majianpeng <majianpeng@xxxxxxxxx> wrote:
>> [snip]
>> I think this patch can do work:
>> Those case which i tested
>> A: filesize=0, buffer=1M
>> B: data[2M] | hole| data[2M], bs= 6M/7M
>
>I don't think your zero buffer change is correct for this test case.
>
>> C: data[4m] | hole | hole |data[2M] bs=16M/18M
>>
>> Are there some case ignore?
>> Thanks!
>> Jianpeng Ma
>>
>>
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index 2ddf061..96ce893 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -319,7 +319,7 @@ static int striped_read(struct inode *inode,
>> int read;
>> struct page **page_pos;
>> int ret;
>> - bool hit_stripe, was_short;
>> + bool hit_stripe, was_short, hit_hole = false;
>>
>> /*
>> * we may need to do multiple reads. not atomic, unfortunately.
>> @@ -342,21 +342,30 @@ more:
>> ci->i_truncate_seq,
>> ci->i_truncate_size,
>> page_pos, pages_left, page_align);
>> - if (ret == -ENOENT)
>> +
>> + if ((ret == 0 || ret == -ENOENT) && pos < inode->i_size)
>> + hit_hole = true;
>
>why do we need 'hit_hole', it's essential the same as was_short. The code
>is already so messy.
>I don't think adding 'hit_hole' make it more readable.
>
>> + else if (ret == -ENOENT)
>> ret = 0;
>> +
>> hit_stripe = this_len < left;
>> - was_short = ret >= 0 && ret < this_len;
>> - dout("striped_read %llu~%u (read %u) got %d%s%s\n", pos, left,
>read,
>> - ret, hit_stripe ? " HITSTRIPE" : "", was_short ? " SHORT" :
>"");
>> -
>> - if (ret > 0) {
>> - int didpages = (page_align + ret) >> PAGE_CACHE_SHIFT;
>> -
>> - if (read < pos - off) {
>> - dout(" zero gap %llu to %llu\n", off + read, pos);
>> - ceph_zero_page_vector_range(page_align + read,
>> - pos - off - read,
>pages);
>> + was_short = ret > 0 && ret < this_len;
>> + dout("striped_read %llu~%u (read %u) got %d%s%s%s\n", pos, left,
>read,
>> + ret, hit_stripe ? " HITSTRIPE" : "", was_short ? " SHORT" :
>"",
>> + hit_hole ? " HITHOLE" : "");
>> +
>> + if (ret > 0 || hit_hole) {
>> + int didpages;
>> +
>> + if (hit_hole) {
>> + ret = this_len;
>> + dout(" zero hole %llu to %llu\n", pos , pos +
>ret);
>> + ceph_zero_page_vector_range(page_align + read,
>> + ret, pages);
>> + hit_hole = false;
>this is incorrect for the 'ret > 0' and 'ret = -ENOENT' cases.
>
>Besides, if 'pos + this_len >= i_size'. we should do nothing here. let the
>'zero trailing bytes'
>code do the job.
>
>Yan, Zheng
>
>> }
>> + didpages = (page_align + ret) >> PAGE_CACHE_SHIFT;
>> +
>> pos += ret;
>> read = pos - off;
>> left -= ret;
>?韬{.n?????%??檩??w?{.n????u朕?Ф?塄}?财??j:+v?? ? ???2??璀??摺?囤??z夸z罐?+?????w棹f
