Re: [PATCH] libceph: remove the dead dirty path in ceph_put_page_vector()

[Ilya Dryomov]

On Thu, Aug 13, 2026 at 1:41 PM Tal Zussman <tz2294@xxxxxxxxxxxx> wrote:
>
> On 8/13/26 4:11 AM, Ilya Dryomov wrote:
> > On Sun, Aug 9, 2026 at 5:04 PM Tal Zussman <tz2294@xxxxxxxxxxxx> wrote:
> >>
> >> The only caller of ceph_put_page_vector() passes dirty = false, so the
> >> branch is dead code. Remove it along with the parameter. This removes a
> >> call to the deprecated set_page_dirty_lock().
> >>
> >> Signed-off-by: Tal Zussman <tz2294@xxxxxxxxxxxx>
> >> ---
> >>  fs/ceph/addr.c               | 2 +-
> >>  include/linux/ceph/libceph.h | 3 +--
> >>  net/ceph/pagevec.c           | 7 ++-----
> >>  3 files changed, 4 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> >> index e2da3ab9f808..83752f3688fa 100644
> >> --- a/fs/ceph/addr.c
> >> +++ b/fs/ceph/addr.c
> >> @@ -257,7 +257,7 @@ static void finish_netfs_read(struct ceph_osd_request *req)
> >>         if (osd_data->type == CEPH_OSD_DATA_TYPE_PAGES) {
> >>                 ceph_put_page_vector(osd_data->pages,
> >>                                      calc_pages_for(osd_data->alignment,
> >> -                                       osd_data->length), false);
> >> +                                       osd_data->length));
> >>         }
> >>         if (err > 0) {
> >>                 ceph_subvolume_metrics_record_io(fsc->mdsc, ceph_inode(inode),
> >> diff --git a/include/linux/ceph/libceph.h b/include/linux/ceph/libceph.h
> >> index 63e0e2aa1ce9..3098306e6173 100644
> >> --- a/include/linux/ceph/libceph.h
> >> +++ b/include/linux/ceph/libceph.h
> >> @@ -313,8 +313,7 @@ int ceph_wait_for_latest_osdmap(struct ceph_client *client,
> >>
> >>  /* pagevec.c */
> >>  extern void ceph_release_page_vector(struct page **pages, int num_pages);
> >> -extern void ceph_put_page_vector(struct page **pages, int num_pages,
> >> -                                bool dirty);
> >> +void ceph_put_page_vector(struct page **pages, int num_pages);
> >>  extern struct page **ceph_alloc_page_vector(int num_pages, gfp_t flags);
> >>  extern void ceph_copy_from_page_vector(struct page **pages,
> >>                                     void *data,
> >> diff --git a/net/ceph/pagevec.c b/net/ceph/pagevec.c
> >> index 858359873c4d..828b003aa2fc 100644
> >> --- a/net/ceph/pagevec.c
> >> +++ b/net/ceph/pagevec.c
> >> @@ -10,15 +10,12 @@
> >>
> >>  #include <linux/ceph/libceph.h>
> >>
> >> -void ceph_put_page_vector(struct page **pages, int num_pages, bool dirty)
> >> +void ceph_put_page_vector(struct page **pages, int num_pages)
> >>  {
> >>         int i;
> >>
> >> -       for (i = 0; i < num_pages; i++) {
> >> -               if (dirty)
> >> -                       set_page_dirty_lock(pages[i]);
> >> +       for (i = 0; i < num_pages; i++)
> >>                 put_page(pages[i]);
> >> -       }
> >>         kvfree(pages);
> >>  }
> >>  EXPORT_SYMBOL(ceph_put_page_vector);
> >>
> >> ---
> >> base-commit: 30c0913a2b44a66dc17283a88489b3f3e12f4327
> >> change-id: 20260809-ceph-put-page-vector-458bb3aa87f6
> >>
> >> Best regards,
> >> --
> >> Tal Zussman <tz2294@xxxxxxxxxxxx>
> >>
> >
> > Hi Tal,
> >
> > I'd suggest getting rid of ceph_put_page_vector() altogether -- it's
> > pretty confusing to use it to put something that wasn't allocated with
> > some ceph_*_page_vector() helper.  It looks like ceph_put_page_vector()
> > was paired with ceph_get_direct_page_vector() which is long gone.  The
> > put_page() loop and kvfree() could be done in that single caller.
> >
> > As I was looking into this, I think I spotted a related memory leak in
> > ceph_netfs_issue_read().  On the fscrypt path, the "page vector" coming
> > from iov_iter_get_pages_alloc2() isn't put in case of EIO caused by
> > ceph_inc_osd_stopping_blocker().  Alex, please take note.
> >
> > Thanks,
> >
> >                  Ilya
> >
>
> Hi Ilya,
>
> Thanks for the review!
>
> I'm happy to fix the ceph_netfs_issue_read() issue as well in a separate
> patch by using ceph_put_page_vector() there (for backporting purposes) and
> then inlining ceph_put_page_vector() in the (now two) callers. Does that
> sound reasonable to you?

I'd rather see if the ceph_netfs_issue_read() issue can be taken care of
by moving ceph_inc_osd_stopping_blocker() call.  If so, there would only
be one place where the "page vector" needs to be put.

Thanks,

                Ilya