Re: Re: question about striped_read

[majianpeng <majianpeng@gmail.com>]

>On Thu, Aug 1, 2013 at 9:45 AM, majianpeng <majianpeng@gmail.com> wrote:
>>>On Wed, Jul 31, 2013 at 3:32 PM, majianpeng <majianpeng@gmail.com> wrote:
>>>>
>>>>  [snip]
>>>> Test case
>>>> A: touch file
>>>>     dd if=file of=/dev/null bs=5M count=1 iflag=direct
>>>> B: [data(2M)|hole(2m)][data(2M)]
>>>>    dd if=file of=/dev/null bs=8M count=1 iflag=direct
>>>> C: [data(4M)[hole(4M)][hole(4M)][data(2M)]
>>>>   dd if=file of=/dev/null bs=16M count=1 iflag=direct
>>>> D: touch file;truncate -s 5M file
>>>>   dd if=file of=/dev/null bs=8M count=1 iflag=direct
>>>>
>>>> Those cases can work.
>>>> Now i make different processing  for short-read between 'ret > 0' and "ret =0".
>>>> For the short-read which ret > 0, it don't do read-page rather than zero the left area.
>>>> This means reduce one meaningless read operation.
>>>>
>>>
>>>This patch looks good. But I still hope not to duplicate code.
>>>
>>>how about change
>>> "hit_stripe = this_len < left;"
>>>to
>>> "hit_stripe = this_len < left && (ret == this_len || pos + this_len <
>>>inode->i_size);"
>>>
>> To make the code easy to understand, i don't apply your suggestion.But i add this check on the judgement of
>> whether read more contents.
>> The follow is the latest patch.Can you check?
>>
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index 2ddf061..3d8d14d 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -349,44 +349,38 @@ more:
>>         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);
>> +       if (ret >= 0) {
>> +               int  didpages;
>> +               if (was_short && (pos + ret < inode->i_size)) {
>> +                       u64 tmp = min(this_len - ret,
>> +                                inode->i_size - pos - ret);
>> +                       dout(" zero gap %llu to %llu\n",
>> +                               pos + ret, pos + ret + tmp);
>> +                       ceph_zero_page_vector_range(page_align + read + ret,
>> +                                                       tmp, pages);
>> +                       ret += tmp;
>>                 }
>> +
>> +               didpages = (page_align + ret) >> PAGE_CACHE_SHIFT;
>>                 pos += ret;
>>                 read = pos - off;
>>                 left -= ret;
>>                 page_pos += didpages;
>>                 pages_left -= didpages;
>>
>> -               /* hit stripe? */
>> -               if (left && hit_stripe)
>> +               /* hit stripe and need continue*/
>> +               if (left && hit_stripe && pos < inode->i_size)
>>                         goto more;
>> +
>>         }
>>
>> -       if (was_short) {
>> +       if (ret >= 0) {
>> +               ret = read;
>>                 /* did we bounce off eof? */
>>                 if (pos + left > inode->i_size)
>>                         *checkeof = 1;
>> -
>> -               /* zero trailing bytes (inside i_size) */
>> -               if (left > 0 && pos < inode->i_size) {
>> -                       if (pos + left > inode->i_size)
>> -                               left = inode->i_size - pos;
>> -
>> -                       dout("zero tail %d\n", left);
>> -                       ceph_zero_page_vector_range(page_align + read, left,
>> -                                                   pages);
>> -                       read += left;
>> -               }
>>         }
>>
>> -       if (ret >= 0)
>> -               ret = read;
>
>I think this line should be "if (read > 0) ret = read;". Other than
>this, your patch looks good.
Because you metioned this, I noticed for ceph_sync_read/write the result are && the every striped read/write.
That is if we met one error, the total result is error.It can't return partial result.
I think i should write anthor patch for that.
>You can add "Reviewed-by: Yan, Zheng <zheng.z.yan@intel.com>" to your
>formal patch.
Ok ,thanks your times.

Thanks!
Jianpeng Ma
>
>Regards
>Yan, Zheng
>
>
>>         dout("striped_read returns %d\n", ret);
>>         return ret;
>>  }
>>
>> Thanks!
>> Jianpeng Ma
Thanks!
Jianpeng Ma
>On Thu, Aug 1, 2013 at 9:45 AM, majianpeng <majianpeng@gmail.com> wrote:
>>>On Wed, Jul 31, 2013 at 3:32 PM, majianpeng <majianpeng@gmail.com> wrote:
>>>>
>>>>  [snip]
>>>> Test case
>>>> A: touch file
>>>>     dd if=file of=/dev/null bs=5M count=1 iflag=direct
>>>> B: [data(2M)|hole(2m)][data(2M)]
>>>>    dd if=file of=/dev/null bs=8M count=1 iflag=direct
>>>> C: [data(4M)[hole(4M)][hole(4M)][data(2M)]
>>>>   dd if=file of=/dev/null bs=16M count=1 iflag=direct
>>>> D: touch file;truncate -s 5M file
>>>>   dd if=file of=/dev/null bs=8M count=1 iflag=direct
>>>>
>>>> Those cases can work.
>>>> Now i make different processing  for short-read between 'ret > 0' and "ret =0".
>>>> For the short-read which ret > 0, it don't do read-page rather than zero the left area.
>>>> This means reduce one meaningless read operation.
>>>>
>>>
>>>This patch looks good. But I still hope not to duplicate code.
>>>
>>>how about change
>>> "hit_stripe = this_len < left;"
>>>to
>>> "hit_stripe = this_len < left && (ret == this_len || pos + this_len <
>>>inode->i_size);"
>>>
>> To make the code easy to understand, i don't apply your suggestion.But i add this check on the judgement of
>> whether read more contents.
>> The follow is the latest patch.Can you check?
>>
>> diff --git a/fs/ceph/file.c b/fs/ceph/file.c
>> index 2ddf061..3d8d14d 100644
>> --- a/fs/ceph/file.c
>> +++ b/fs/ceph/file.c
>> @@ -349,44 +349,38 @@ more:
>>         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);
>> +       if (ret >= 0) {
>> +               int  didpages;
>> +               if (was_short && (pos + ret < inode->i_size)) {
>> +                       u64 tmp = min(this_len - ret,
>> +                                inode->i_size - pos - ret);
>> +                       dout(" zero gap %llu to %llu\n",
>> +                               pos + ret, pos + ret + tmp);
>> +                       ceph_zero_page_vector_range(page_align + read + ret,
>> +                                                       tmp, pages);
>> +                       ret += tmp;
>>                 }
>> +
>> +               didpages = (page_align + ret) >> PAGE_CACHE_SHIFT;
>>                 pos += ret;
>>                 read = pos - off;
>>                 left -= ret;
>>                 page_pos += didpages;
>>                 pages_left -= didpages;
>>
>> -               /* hit stripe? */
>> -               if (left && hit_stripe)
>> +               /* hit stripe and need continue*/
>> +               if (left && hit_stripe && pos < inode->i_size)
>>                         goto more;
>> +
>>         }
>>
>> -       if (was_short) {
>> +       if (ret >= 0) {
>> +               ret = read;
>>                 /* did we bounce off eof? */
>>                 if (pos + left > inode->i_size)
>>                         *checkeof = 1;
>> -
>> -               /* zero trailing bytes (inside i_size) */
>> -               if (left > 0 && pos < inode->i_size) {
>> -                       if (pos + left > inode->i_size)
>> -                               left = inode->i_size - pos;
>> -
>> -                       dout("zero tail %d\n", left);
>> -                       ceph_zero_page_vector_range(page_align + read, left,
>> -                                                   pages);
>> -                       read += left;
>> -               }
>>         }
>>
>> -       if (ret >= 0)
>> -               ret = read;
>
>I think this line should be "if (read > 0) ret = read;". Other than
>this, your patch looks good.
>You can add "Reviewed-by: Yan, Zheng <zheng.z.yan@intel.com>" to your
>formal patch.
>
>Regards
>Yan, Zheng
>
>
>>         dout("striped_read returns %d\n", ret);
>>         return ret;
>>  }
>>
>> Thanks!
>> Jianpeng Ma