Re: Re: question about striped_read

[majianpeng <majianpeng@gmail.com>]

>On Wed, Jul 31, 2013 at 9:36 AM, majianpeng <majianpeng@gmail.com> 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@gmail.com> 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;
>