* [PATCH v2 0/2] ceph: skip copying the data extends the file EOF @ 2024-02-22 13:18 xiubli 2024-02-22 13:18 ` [PATCH v2 1/2] " xiubli 2024-02-22 13:19 ` [PATCH v2 2/2] ceph: set the correct mask for getattr reqeust for read xiubli 0 siblings, 2 replies; 7+ messages in thread From: xiubli @ 2024-02-22 13:18 UTC (permalink / raw) To: ceph-devel; +Cc: idryomov, jlayton, vshankar, mchangir, Xiubo Li From: Xiubo Li <xiubli@redhat.com> V2: - append a new patch to improve the getattr code Xiubo Li (2): ceph: skip copying the data extends the file EOF ceph: set the correct mask for getattr reqeust for read fs/ceph/file.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-) -- 2.43.0 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] ceph: skip copying the data extends the file EOF 2024-02-22 13:18 [PATCH v2 0/2] ceph: skip copying the data extends the file EOF xiubli @ 2024-02-22 13:18 ` xiubli 2024-02-26 3:45 ` 回覆: " Frank Hsiao 蕭法宣 2024-03-18 21:02 ` Ilya Dryomov 2024-02-22 13:19 ` [PATCH v2 2/2] ceph: set the correct mask for getattr reqeust for read xiubli 1 sibling, 2 replies; 7+ messages in thread From: xiubli @ 2024-02-22 13:18 UTC (permalink / raw) To: ceph-devel Cc: idryomov, jlayton, vshankar, mchangir, Xiubo Li, Frank Hsiao 蕭法宣 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; 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) -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* 回覆: [PATCH v2 1/2] ceph: skip copying the data extends the file EOF 2024-02-22 13:18 ` [PATCH v2 1/2] " xiubli @ 2024-02-26 3:45 ` Frank Hsiao 蕭法宣 2024-03-18 21:02 ` Ilya Dryomov 1 sibling, 0 replies; 7+ messages in thread From: Frank Hsiao 蕭法宣 @ 2024-02-26 3:45 UTC (permalink / raw) To: xiubli@redhat.com; +Cc: ceph-devel@vger.kernel.org Tested-by: Frank Hsiao 蕭法宣 <frankhsiao@qnap.com> ________________________________________ 寄件者: xiubli@redhat.com <xiubli@redhat.com> 寄件日期: 2024年2月22日 下午 09:18 收件者: ceph-devel@vger.kernel.org 副本: idryomov@gmail.com; jlayton@kernel.org; vshankar@redhat.com; mchangir@redhat.com; Xiubo Li; Frank Hsiao 蕭法宣 主旨: [PATCH v2 1/2] ceph: skip copying the data extends the file EOF 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; 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) -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ceph: skip copying the data extends the file EOF 2024-02-22 13:18 ` [PATCH v2 1/2] " xiubli 2024-02-26 3:45 ` 回覆: " Frank Hsiao 蕭法宣 @ 2024-03-18 21:02 ` Ilya Dryomov 2024-03-19 0:18 ` Xiubo Li 1 sibling, 1 reply; 7+ messages in thread From: Ilya Dryomov @ 2024-03-18 21:02 UTC (permalink / raw) To: xiubli Cc: ceph-devel, jlayton, vshankar, mchangir, Frank Hsiao 蕭法宣 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] ceph: skip copying the data extends the file EOF 2024-03-18 21:02 ` Ilya Dryomov @ 2024-03-19 0:18 ` Xiubo Li 0 siblings, 0 replies; 7+ messages in thread From: Xiubo Li @ 2024-03-19 0:18 UTC (permalink / raw) To: Ilya Dryomov Cc: ceph-devel, jlayton, vshankar, mchangir, Frank Hsiao 蕭法宣 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 > ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] ceph: set the correct mask for getattr reqeust for read 2024-02-22 13:18 [PATCH v2 0/2] ceph: skip copying the data extends the file EOF xiubli 2024-02-22 13:18 ` [PATCH v2 1/2] " xiubli @ 2024-02-22 13:19 ` xiubli 2024-02-26 3:46 ` 回覆: " Frank Hsiao 蕭法宣 1 sibling, 1 reply; 7+ messages in thread From: xiubli @ 2024-02-22 13:19 UTC (permalink / raw) To: ceph-devel Cc: idryomov, jlayton, vshankar, mchangir, Xiubo Li, Frank Hsiao 蕭法宣 From: Xiubo Li <xiubli@redhat.com> In case of hitting the file EOF the ceph_read_iter() needs to retrieve the file size from MDS, and the Fr caps is not a neccessary. 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 | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 2b2b07a0a61b..08c918aa403e 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -2181,14 +2181,16 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) int statret; struct page *page = NULL; loff_t i_size; + int mask = CEPH_STAT_CAP_SIZE; if (retry_op == READ_INLINE) { page = __page_cache_alloc(GFP_KERNEL); if (!page) return -ENOMEM; } - statret = __ceph_do_getattr(inode, page, - CEPH_STAT_CAP_INLINE_DATA, !!page); + if (retry_op == READ_INLINE) + mask = CEPH_STAT_CAP_INLINE_DATA; + statret = __ceph_do_getattr(inode, page, mask, !!page); if (statret < 0) { if (page) __free_page(page); @@ -2229,7 +2231,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) /* hit EOF or hole? */ if (retry_op == CHECK_EOF && iocb->ki_pos < i_size && ret < len) { - doutc(cl, "hit hole, ppos %lld < size %lld, reading more\n", + doutc(cl, "may hit hole, ppos %lld < size %lld, reading more\n", iocb->ki_pos, i_size); read += ret; -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* 回覆: [PATCH v2 2/2] ceph: set the correct mask for getattr reqeust for read 2024-02-22 13:19 ` [PATCH v2 2/2] ceph: set the correct mask for getattr reqeust for read xiubli @ 2024-02-26 3:46 ` Frank Hsiao 蕭法宣 0 siblings, 0 replies; 7+ messages in thread From: Frank Hsiao 蕭法宣 @ 2024-02-26 3:46 UTC (permalink / raw) To: xiubli@redhat.com; +Cc: ceph-devel@vger.kernel.org Tested-by: Frank Hsiao 蕭法宣 <frankhsiao@qnap.com> ________________________________________ 寄件者: xiubli@redhat.com <xiubli@redhat.com> 寄件日期: 2024年2月22日 下午 09:19 收件者: ceph-devel@vger.kernel.org 副本: idryomov@gmail.com; jlayton@kernel.org; vshankar@redhat.com; mchangir@redhat.com; Xiubo Li; Frank Hsiao 蕭法宣 主旨: [PATCH v2 2/2] ceph: set the correct mask for getattr reqeust for read From: Xiubo Li <xiubli@redhat.com> In case of hitting the file EOF the ceph_read_iter() needs to retrieve the file size from MDS, and the Fr caps is not a neccessary. 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 | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 2b2b07a0a61b..08c918aa403e 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -2181,14 +2181,16 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) int statret; struct page *page = NULL; loff_t i_size; + int mask = CEPH_STAT_CAP_SIZE; if (retry_op == READ_INLINE) { page = __page_cache_alloc(GFP_KERNEL); if (!page) return -ENOMEM; } - statret = __ceph_do_getattr(inode, page, - CEPH_STAT_CAP_INLINE_DATA, !!page); + if (retry_op == READ_INLINE) + mask = CEPH_STAT_CAP_INLINE_DATA; + statret = __ceph_do_getattr(inode, page, mask, !!page); if (statret < 0) { if (page) __free_page(page); @@ -2229,7 +2231,7 @@ static ssize_t ceph_read_iter(struct kiocb *iocb, struct iov_iter *to) /* hit EOF or hole? */ if (retry_op == CHECK_EOF && iocb->ki_pos < i_size && ret < len) { - doutc(cl, "hit hole, ppos %lld < size %lld, reading more\n", + doutc(cl, "may hit hole, ppos %lld < size %lld, reading more\n", iocb->ki_pos, i_size); read += ret; -- 2.43.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-03-19 0:18 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-22 13:18 [PATCH v2 0/2] ceph: skip copying the data extends the file EOF xiubli 2024-02-22 13:18 ` [PATCH v2 1/2] " xiubli 2024-02-26 3:45 ` 回覆: " Frank Hsiao 蕭法宣 2024-03-18 21:02 ` Ilya Dryomov 2024-03-19 0:18 ` Xiubo Li 2024-02-22 13:19 ` [PATCH v2 2/2] ceph: set the correct mask for getattr reqeust for read xiubli 2024-02-26 3:46 ` 回覆: " Frank Hsiao 蕭法宣
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).