| Message ID | 20211020132813.543695-4-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: size handling for the fscrypt | expand |
On Wed, 2021-10-20 at 21:28 +0800, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > Signed-off-by: Xiubo Li <xiubli@redhat.com> > --- > fs/ceph/file.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 74db403a4c35..1988e75ad4a2 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -910,6 +910,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > ssize_t ret; > u64 off = *ki_pos; > u64 len = iov_iter_count(to); > + u64 i_size = i_size_read(inode); > > dout("sync_read on inode %p %llu~%u\n", inode, *ki_pos, (unsigned)len); > > @@ -933,7 +934,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > struct page **pages; > int num_pages; > size_t page_off; > - u64 i_size; > bool more; > int idx; > size_t left; > @@ -980,7 +980,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > > ceph_osdc_put_request(req); > > - i_size = i_size_read(inode); > dout("sync_read %llu~%llu got %zd i_size %llu%s\n", > off, len, ret, i_size, (more ? " MORE" : "")); > > @@ -1056,11 +1055,14 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > } > > if (off > *ki_pos) { > - if (ret >= 0 && > - iov_iter_count(to) > 0 && off >= i_size_read(inode)) > + if (off >= i_size) { > *retry_op = CHECK_EOF; > - ret = off - *ki_pos; > - *ki_pos = off; > + ret = i_size - *ki_pos; > + *ki_pos = i_size; > + } else { > + ret = off - *ki_pos; > + *ki_pos = off; > + } > } > out: > dout("sync_read result %zd retry_op %d\n", ret, *retry_op); I'm guessing this is fixing a bug? Did you hit this in testing or did you just notice by inspection? Should we merge this in advance of the rest of the set?
On 10/26/21 3:05 AM, Jeff Layton wrote: > On Wed, 2021-10-20 at 21:28 +0800, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> Signed-off-by: Xiubo Li <xiubli@redhat.com> >> --- >> fs/ceph/file.c | 14 ++++++++------ >> 1 file changed, 8 insertions(+), 6 deletions(-) >> >> diff --git a/fs/ceph/file.c b/fs/ceph/file.c >> index 74db403a4c35..1988e75ad4a2 100644 >> --- a/fs/ceph/file.c >> +++ b/fs/ceph/file.c >> @@ -910,6 +910,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, >> ssize_t ret; >> u64 off = *ki_pos; >> u64 len = iov_iter_count(to); >> + u64 i_size = i_size_read(inode); >> >> dout("sync_read on inode %p %llu~%u\n", inode, *ki_pos, (unsigned)len); >> >> @@ -933,7 +934,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, >> struct page **pages; >> int num_pages; >> size_t page_off; >> - u64 i_size; >> bool more; >> int idx; >> size_t left; >> @@ -980,7 +980,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, >> >> ceph_osdc_put_request(req); >> >> - i_size = i_size_read(inode); >> dout("sync_read %llu~%llu got %zd i_size %llu%s\n", >> off, len, ret, i_size, (more ? " MORE" : "")); >> >> @@ -1056,11 +1055,14 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, >> } >> >> if (off > *ki_pos) { >> - if (ret >= 0 && >> - iov_iter_count(to) > 0 && off >= i_size_read(inode)) >> + if (off >= i_size) { >> *retry_op = CHECK_EOF; >> - ret = off - *ki_pos; >> - *ki_pos = off; >> + ret = i_size - *ki_pos; >> + *ki_pos = i_size; >> + } else { >> + ret = off - *ki_pos; >> + *ki_pos = off; >> + } >> } >> out: >> dout("sync_read result %zd retry_op %d\n", ret, *retry_op); > I'm guessing this is fixing a bug? Did you hit this in testing or did > you just notice by inspection? Should we merge this in advance of the > rest of the set? This is one bug, when testing the fscrypt feature and I can reproduce it every time, in theory this bug should be seen in none fscrypt case. I just send the V2 here in this series to show in which use case I hit this bug. If it looks good I will post the V3 based the upstream code and add more comments on it.
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 74db403a4c35..1988e75ad4a2 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -910,6 +910,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, ssize_t ret; u64 off = *ki_pos; u64 len = iov_iter_count(to); + u64 i_size = i_size_read(inode); dout("sync_read on inode %p %llu~%u\n", inode, *ki_pos, (unsigned)len); @@ -933,7 +934,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, struct page **pages; int num_pages; size_t page_off; - u64 i_size; bool more; int idx; size_t left; @@ -980,7 +980,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, ceph_osdc_put_request(req); - i_size = i_size_read(inode); dout("sync_read %llu~%llu got %zd i_size %llu%s\n", off, len, ret, i_size, (more ? " MORE" : "")); @@ -1056,11 +1055,14 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, } if (off > *ki_pos) { - if (ret >= 0 && - iov_iter_count(to) > 0 && off >= i_size_read(inode)) + if (off >= i_size) { *retry_op = CHECK_EOF; - ret = off - *ki_pos; - *ki_pos = off; + ret = i_size - *ki_pos; + *ki_pos = i_size; + } else { + ret = off - *ki_pos; + *ki_pos = off; + } } out: dout("sync_read result %zd retry_op %d\n", ret, *retry_op);