| Message ID | 20240222131900.179717-2-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: skip copying the data extends the file EOF | expand |
On Thu, Feb 22, 2024 at 2:21 PM <xiubli@redhat.com> wrote: > > From: Xiubo Li <xiubli@redhat.com> > > If hits the EOF it will revise the return value to the i_size > instead of the real length read, but it will advance the offset > of iovc, then for the next try it may be incorrectly skipped. > > This will just skip advancing the iovc's offset more than i_size. > > URL: https://patchwork.kernel.org/project/ceph-devel/list/?series=819323 > Reported-by: Frank Hsiao 蕭法宣 <frankhsiao@qnap.com> > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/file.c | 18 ++++++++---------- > 1 file changed, 8 insertions(+), 10 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 71d29571712d..2b2b07a0a61b 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1195,7 +1195,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > } > > idx = 0; > - left = ret > 0 ? ret : 0; > + left = ret > 0 ? umin(ret, i_size) : 0; Hi Xiubo, Can ret (i.e. the number of bytes actually read) be compared to i_size without taking the offset into account? How does this a handle a case where e.g. off = 20 ret = 10 i_size = 25 Did you intend the copy_page_to_iter() loop to go over 10 bytes and therefore advance the iovc ("to") by 10 instead of 5 bytes here? Thanks, Ilya
On 3/19/24 05:02, Ilya Dryomov wrote: > On Thu, Feb 22, 2024 at 2:21 PM <xiubli@redhat.com> wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> If hits the EOF it will revise the return value to the i_size >> instead of the real length read, but it will advance the offset >> of iovc, then for the next try it may be incorrectly skipped. >> >> This will just skip advancing the iovc's offset more than i_size. >> >> URL: https://patchwork.kernel.org/project/ceph-devel/list/?series=819323 >> Reported-by: Frank Hsiao 蕭法宣 <frankhsiao@qnap.com> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/file.c | 18 ++++++++---------- >> 1 file changed, 8 insertions(+), 10 deletions(-) >> >> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >> index 71d29571712d..2b2b07a0a61b 100644 >> --- a/fs/ceph/file.c >> +++ b/fs/ceph/file.c >> @@ -1195,7 +1195,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, >> } >> >> idx = 0; >> - left = ret > 0 ? ret : 0; >> + left = ret > 0 ? umin(ret, i_size) : 0; > Hi Xiubo, > > Can ret (i.e. the number of bytes actually read) be compared to i_size > without taking the offset into account? How does this a handle a case > where e.g. > > off = 20 > ret = 10 > i_size = 25 > > Did you intend the copy_page_to_iter() loop to go over 10 bytes and > therefore advance the iovc ("to") by 10 instead of 5 bytes here? Good catch! diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 142deb242196..531874935c21 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1204,7 +1204,12 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, } idx = 0; - left = ret > 0 ? umin(ret, i_size) : 0; + if (ret <= 0) + left = 0; + else if (off + ret > i_size) + left = i_size - off; + else + left = ret; while (left > 0) { size_t plen, copied; Let me fix this in V3. Thanks Ilya! - Xiubo > Thanks, > > Ilya >
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 71d29571712d..2b2b07a0a61b 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1195,7 +1195,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, } idx = 0; - left = ret > 0 ? ret : 0; + left = ret > 0 ? umin(ret, i_size) : 0; while (left > 0) { size_t plen, copied; @@ -1224,15 +1224,13 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, } if (ret > 0) { - if (off > *ki_pos) { - if (off >= i_size) { - *retry_op = CHECK_EOF; - ret = i_size - *ki_pos; - *ki_pos = i_size; - } else { - ret = off - *ki_pos; - *ki_pos = off; - } + if (off >= i_size) { + *retry_op = CHECK_EOF; + ret = i_size - *ki_pos; + *ki_pos = i_size; + } else { + ret = off - *ki_pos; + *ki_pos = off; } if (last_objver)