diff mbox series

[v2,1/2] ceph: skip copying the data extends the file EOF

Message ID 20240222131900.179717-2-xiubli@redhat.com
State New
Headers show
Series ceph: skip copying the data extends the file EOF | expand

Commit Message

Xiubo Li Feb. 22, 2024, 1:18 p.m. UTC
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(-)

Comments

Ilya Dryomov March 18, 2024, 9:02 p.m. UTC | #1
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
Xiubo Li March 19, 2024, 12:18 a.m. UTC | #2
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 mbox series

Patch

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)