| Message ID | 201308010929458493571@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>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
On Thu, Aug 1, 2013 at 2:30 PM, majianpeng <majianpeng@gmail.com> wrote: >>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. This behavior is not correct. If we read/write some data, then meet an error, we should return the size we have read/written. I think all other FS behave like this. See generic_file_aio_read() and do_generic_file_read(). Regards Yan, Zheng > 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 -- To unsubscribe from this list: send the line "unsubscribe ceph-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; dout("striped_read returns %d\n", ret); return ret; }