On Thu, Aug 1, 2013 at 9:45 AM, majianpeng <majianpeng@xxxxxxxxx> wrote:
>>On Wed, Jul 31, 2013 at 3:32 PM, majianpeng <majianpeng@xxxxxxxxx> 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@xxxxxxxxx>" to your
formal patch.
Regards
Yan, Zheng
> dout("striped_read returns %d\n", ret);
> return ret;
> }
>
> Thanks!
> Jianpeng Ma
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
