| Message ID | 20241205154951.4163232-1-max.kellermann@ionos.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fs/ceph/file: fix memory leaks in __ceph_sync_read() | expand |
Good. This sequence has not been tested independently, but it should be fine. On Thu, Dec 5, 2024 at 5:49 PM Max Kellermann <max.kellermann@ionos.com> wrote: > > In two `break` statements, the call to ceph_release_page_vector() was > missing, leaking the allocation from ceph_alloc_page_vector(). > > Instead of adding the missing ceph_release_page_vector() calls, the > Ceph maintainers preferred to transfer page ownership to the > `ceph_osd_request` by passing `own_pages=true` to > osd_req_op_extent_osd_data_pages(). This requires postponing the > ceph_osdc_put_request() call until after the block that accesses the > `pages`. > > Cc: stable@vger.kernel.org > Signed-off-by: Max Kellermann <max.kellermann@ionos.com> > --- > fs/ceph/file.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > index 4b8d59ebda00..ce342a5d4b8b 100644 > --- a/fs/ceph/file.c > +++ b/fs/ceph/file.c > @@ -1127,7 +1127,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > > osd_req_op_extent_osd_data_pages(req, 0, pages, read_len, > offset_in_page(read_off), > - false, false); > + false, true); > > op = &req->r_ops[0]; > if (sparse) { > @@ -1186,8 +1186,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > ret = min_t(ssize_t, fret, len); > } > > - ceph_osdc_put_request(req); > - > /* Short read but not EOF? Zero out the remainder. */ > if (ret >= 0 && ret < len && (off + ret < i_size)) { > int zlen = min(len - ret, i_size - off - ret); > @@ -1221,7 +1219,8 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > break; > } > } > - ceph_release_page_vector(pages, num_pages); > + > + ceph_osdc_put_request(req); > > if (ret < 0) { > if (ret == -EBLOCKLISTED) > -- > 2.45.2 >
On Thu, Dec 5, 2024 at 5:30 PM Alex Markuze <amarkuze@redhat.com> wrote: > > Good. > This sequence has not been tested independently, but it should be fine. Applied. Thanks, Ilya > > On Thu, Dec 5, 2024 at 5:49 PM Max Kellermann <max.kellermann@ionos.com> wrote: > > > > In two `break` statements, the call to ceph_release_page_vector() was > > missing, leaking the allocation from ceph_alloc_page_vector(). > > > > Instead of adding the missing ceph_release_page_vector() calls, the > > Ceph maintainers preferred to transfer page ownership to the > > `ceph_osd_request` by passing `own_pages=true` to > > osd_req_op_extent_osd_data_pages(). This requires postponing the > > ceph_osdc_put_request() call until after the block that accesses the > > `pages`. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Max Kellermann <max.kellermann@ionos.com> > > --- > > fs/ceph/file.c | 7 +++---- > > 1 file changed, 3 insertions(+), 4 deletions(-) > > > > diff --git a/fs/ceph/file.c b/fs/ceph/file.c > > index 4b8d59ebda00..ce342a5d4b8b 100644 > > --- a/fs/ceph/file.c > > +++ b/fs/ceph/file.c > > @@ -1127,7 +1127,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > > > > osd_req_op_extent_osd_data_pages(req, 0, pages, read_len, > > offset_in_page(read_off), > > - false, false); > > + false, true); > > > > op = &req->r_ops[0]; > > if (sparse) { > > @@ -1186,8 +1186,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > > ret = min_t(ssize_t, fret, len); > > } > > > > - ceph_osdc_put_request(req); > > - > > /* Short read but not EOF? Zero out the remainder. */ > > if (ret >= 0 && ret < len && (off + ret < i_size)) { > > int zlen = min(len - ret, i_size - off - ret); > > @@ -1221,7 +1219,8 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, > > break; > > } > > } > > - ceph_release_page_vector(pages, num_pages); > > + > > + ceph_osdc_put_request(req); > > > > if (ret < 0) { > > if (ret == -EBLOCKLISTED) > > -- > > 2.45.2 > > >
diff --git a/fs/ceph/file.c b/fs/ceph/file.c index 4b8d59ebda00..ce342a5d4b8b 100644 --- a/fs/ceph/file.c +++ b/fs/ceph/file.c @@ -1127,7 +1127,7 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, osd_req_op_extent_osd_data_pages(req, 0, pages, read_len, offset_in_page(read_off), - false, false); + false, true); op = &req->r_ops[0]; if (sparse) { @@ -1186,8 +1186,6 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, ret = min_t(ssize_t, fret, len); } - ceph_osdc_put_request(req); - /* Short read but not EOF? Zero out the remainder. */ if (ret >= 0 && ret < len && (off + ret < i_size)) { int zlen = min(len - ret, i_size - off - ret); @@ -1221,7 +1219,8 @@ ssize_t __ceph_sync_read(struct inode *inode, loff_t *ki_pos, break; } } - ceph_release_page_vector(pages, num_pages); + + ceph_osdc_put_request(req); if (ret < 0) { if (ret == -EBLOCKLISTED)
In two `break` statements, the call to ceph_release_page_vector() was missing, leaking the allocation from ceph_alloc_page_vector(). Instead of adding the missing ceph_release_page_vector() calls, the Ceph maintainers preferred to transfer page ownership to the `ceph_osd_request` by passing `own_pages=true` to osd_req_op_extent_osd_data_pages(). This requires postponing the ceph_osdc_put_request() call until after the block that accesses the `pages`. Cc: stable@vger.kernel.org Signed-off-by: Max Kellermann <max.kellermann@ionos.com> --- fs/ceph/file.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)