On Sun, 2024-12-08 at 17:32 +0000, Alex Markuze wrote: > This patch refines the read logic in __ceph_sync_read() to ensure > more > predictable and efficient behavior in various edge cases. > > - Return early if the requested read length is zero or if the file > size > (`i_size`) is zero. > - Initialize the index variable (`idx`) where needed and reorder some > code to ensure it is always set before use. > - Improve error handling by checking for negative return values > earlier. > - Remove redundant encrypted file checks after failures. Only attempt > filesystem-level decryption if the read succeeded. > - Simplify leftover calculations to correctly handle cases where the > read > extends beyond the end of the file or stops short. > - This resolves multiple issues caused by integer overflow > - > https://tracker.ceph.com/issues/67524 > > - > https://tracker.ceph.com/issues/68981 > > - > https://tracker.ceph.com/issues/68980 > > > Signed-off-by: Alex Markuze <amarkuze@xxxxxxxxxx> > --- > fs/ceph/file.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index ce342a5d4b8b..8e0400d461a2 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1066,7 +1066,7 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > if (ceph_inode_is_shutdown(inode)) > return -EIO; > > - if (!len) > + if (!len || !i_size) > return 0; > /* > * flush any page cache pages in this range. this > @@ -1086,7 +1086,7 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > int num_pages; > size_t page_off; > bool more; > - int idx; > + int idx = 0; > size_t left; > struct ceph_osd_req_op *op; > u64 read_off = off; > @@ -1160,7 +1160,14 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > else if (ret == -ENOENT) > ret = 0; > > - if (ret > 0 && IS_ENCRYPTED(inode)) { > + if (ret < 0) { If I understood correctly, you simply added the check for error in the execution flow (ret < 0) before processing IS_ENCRYPTED(inode) condition. Am I correct here? Thanks, Slava. > + ceph_osdc_put_request(req); > + if (ret == -EBLOCKLISTED) > + fsc->blocklisted = true; > + break; > + } > + > + if (IS_ENCRYPTED(inode)) { > int fret; > > fret = ceph_fscrypt_decrypt_extents(inode, > pages, > @@ -1187,7 +1194,7 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > } > > /* Short read but not EOF? Zero out the remainder. > */ > - if (ret >= 0 && ret < len && (off + ret < i_size)) { > + if (ret < len && (off + ret < i_size)) { > int zlen = min(len - ret, i_size - off - > ret); > int zoff = page_off + ret; > > @@ -1197,13 +1204,11 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > ret += zlen; > } > > - idx = 0; > - if (ret <= 0) > - left = 0; > - else if (off + ret > i_size) > - left = i_size - off; > + if (off + ret > i_size) > + left = (i_size > off) ? i_size - off : 0; > else > left = ret; > + > while (left > 0) { > size_t plen, copied; > > @@ -1222,12 +1227,6 @@ ssize_t __ceph_sync_read(struct inode *inode, > loff_t *ki_pos, > > ceph_osdc_put_request(req); > > - if (ret < 0) { > - if (ret == -EBLOCKLISTED) > - fsc->blocklisted = true; > - break; > - } > - > if (off >= i_size || !more) > break; > } > -- > 2.34.1
