[PATCH] libceph: use iov_iter_extract_pages() in ceph_msg_data_iter_next()

[PATCH] libceph: use iov_iter_extract_pages() in ceph_msg_data_iter_next()

[Tal Zussman]

ceph_msg_data_iter_next() gets a page reference from
iov_iter_get_pages2() only to immediately drop it, asserting that the
page is pinned some other way. The FIXME here predates
iov_iter_extract_pages(), which takes no reference for kernel-backed
iterators.

CEPH_MSG_DATA_ITER data only comes from osd_req_op_extent_osd_iter(),
whose only caller passes the netfs read iterator, which is always
kernel-backed. Use iov_iter_extract_pages() and remove the put and the
assertion. The messenger still relies on the upper layers to keep the
pages alive while it uses them, as it did before. Extracting from a
user-backed iterator would pin pages that nothing unpins, so add a
precautionary warn in ceph_msg_data_add_iter().

This removes the last caller of PageWriteback(), allowing the page
flag accessors to be removed in a future patch.

Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
The assertion is the last caller of PageWriteback() in the tree. The
removal of the PG_writeback page flag accessors will be sent
separately.
---
 net/ceph/messenger.c | 20 ++++++--------------
 1 file changed, 6 insertions(+), 14 deletions(-)

diff --git a/net/ceph/messenger.c b/net/ceph/messenger.c
index fd9c9e64dc8a..e08c3330b9f2 100644
--- a/net/ceph/messenger.c
+++ b/net/ceph/messenger.c
@@ -996,28 +996,17 @@ static void ceph_msg_data_iter_cursor_init(struct ceph_msg_data_cursor *cursor,
 static struct page *ceph_msg_data_iter_next(struct ceph_msg_data_cursor *cursor,
 					    size_t *page_offset, size_t *length)
 {
-	struct page *page;
+	struct page *page, **ppage = &page;
 	ssize_t len;
 
 	if (cursor->lastlen)
 		iov_iter_revert(&cursor->iov_iter, cursor->lastlen);
 
-	len = iov_iter_get_pages2(&cursor->iov_iter, &page, PAGE_SIZE,
-				  1, page_offset);
+	len = iov_iter_extract_pages(&cursor->iov_iter, &ppage, PAGE_SIZE,
+				     1, 0, page_offset);
 	BUG_ON(len < 0);
 
 	cursor->lastlen = len;
-
-	/*
-	 * FIXME: The assumption is that the pages represented by the iov_iter
-	 *	  are pinned, with the references held by the upper-level
-	 *	  callers, or by virtue of being under writeback. Eventually,
-	 *	  we'll get an iov_iter_get_pages2 variant that doesn't take
-	 *	  page refs. Until then, just put the page ref.
-	 */
-	VM_BUG_ON_PAGE(!PageWriteback(page) && page_count(page) < 2, page);
-	put_page(page);
-
 	*length = min_t(size_t, len, cursor->resid);
 	return page;
 }
@@ -1967,6 +1956,9 @@ void ceph_msg_data_add_iter(struct ceph_msg *msg,
 {
 	struct ceph_msg_data *data;
 
+	/* the messenger never unpins pages, so the iterator must not pin them */
+	WARN_ON_ONCE(iov_iter_extract_will_pin(iter));
+
 	data = ceph_msg_data_add(msg);
 	data->type = CEPH_MSG_DATA_ITER;
 	data->iter = *iter;

---
base-commit: fce4f3da41f1145cfa400002ce3a66e4a89a1902
change-id: 20260808-ceph-msgr-writeback-a67d25e28f1e

Best regards,
-- 
Tal Zussman <tz2294@columbia.edu>

Re: [PATCH] libceph: use iov_iter_extract_pages() in ceph_msg_data_iter_next()

[Christoph Hellwig]

On Mon, Aug 17, 2026 at 06:16:56PM -0400, Tal Zussman wrote:
> ceph_msg_data_iter_next() gets a page reference from
> iov_iter_get_pages2() only to immediately drop it, asserting that the
> page is pinned some other way. The FIXME here predates
> iov_iter_extract_pages(), which takes no reference for kernel-backed
> iterators.
> 
> CEPH_MSG_DATA_ITER data only comes from osd_req_op_extent_osd_iter(),
> whose only caller passes the netfs read iterator, which is always
> kernel-backed. Use iov_iter_extract_pages() and remove the put and the
> assertion. The messenger still relies on the upper layers to keep the
> pages alive while it uses them, as it did before. Extracting from a
> user-backed iterator would pin pages that nothing unpins, so add a
> precautionary warn in ceph_msg_data_add_iter().

Yikes, this goes through like three layers of pointless abstraction
for a single user :(

But trying to unwind those it comes from ceph_netfs_issue_read, which
is the netfs issue_read method, which is used for all kinds of
reads, but it does seem like for direct reads it uses the kinda
interesting netfs_extract_user_iter helper to turn the user iov
into a kernel one, which makes all of this such a freakin' mess.

So yeah, the analysis is right, at the same time using
iov_iter_extract_pages is just as weird as the old version.  Someone
really needs to clean up all the mess in both netfs and ceph :(

> This removes the last caller of PageWriteback(), allowing the page
> flag accessors to be removed in a future patch.
> 
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
> ---
> The assertion is the last caller of PageWriteback() in the tree. The
> removal of the PG_writeback page flag accessors will be sent
> separately.

What about just killing that assert and leaving the rest of this
mess in place until it is sorted out properly?  iov_iter_get_pages2
is a pretty good marker for that, and it would be sad to loose that.

Re: [PATCH] libceph: use iov_iter_extract_pages() in ceph_msg_data_iter_next()

[Tal Zussman]

On 8/19/26 1:44 AM, Christoph Hellwig wrote:
> On Mon, Aug 17, 2026 at 06:16:56PM -0400, Tal Zussman wrote:
>> ceph_msg_data_iter_next() gets a page reference from
>> iov_iter_get_pages2() only to immediately drop it, asserting that the
>> page is pinned some other way. The FIXME here predates
>> iov_iter_extract_pages(), which takes no reference for kernel-backed
>> iterators.
>> 
>> CEPH_MSG_DATA_ITER data only comes from osd_req_op_extent_osd_iter(),
>> whose only caller passes the netfs read iterator, which is always
>> kernel-backed. Use iov_iter_extract_pages() and remove the put and the
>> assertion. The messenger still relies on the upper layers to keep the
>> pages alive while it uses them, as it did before. Extracting from a
>> user-backed iterator would pin pages that nothing unpins, so add a
>> precautionary warn in ceph_msg_data_add_iter().
> 
> Yikes, this goes through like three layers of pointless abstraction
> for a single user :(
> 
> But trying to unwind those it comes from ceph_netfs_issue_read, which
> is the netfs issue_read method, which is used for all kinds of
> reads, but it does seem like for direct reads it uses the kinda
> interesting netfs_extract_user_iter helper to turn the user iov
> into a kernel one, which makes all of this such a freakin' mess.
> 
> So yeah, the analysis is right, at the same time using
> iov_iter_extract_pages is just as weird as the old version.  Someone
> really needs to clean up all the mess in both netfs and ceph :(
> 
>> This removes the last caller of PageWriteback(), allowing the page
>> flag accessors to be removed in a future patch.
>> 
>> Signed-off-by: Tal Zussman <tz2294@columbia.edu>
>> ---
>> The assertion is the last caller of PageWriteback() in the tree. The
>> removal of the PG_writeback page flag accessors will be sent
>> separately.
> 
> What about just killing that assert and leaving the rest of this
> mess in place until it is sorted out properly?  iov_iter_get_pages2
> is a pretty good marker for that, and it would be sad to loose that.
> 

I'm perfectly happy to do that. I just want the PageWriteback() gone
and wanted to try to fix it properly rather than just delete the BUG()
or throw in a page_folio(). But seems like "properly" in this case may
be a deeper hole than I want to jump in to right now ;)

Re: [PATCH] libceph: use iov_iter_extract_pages() in ceph_msg_data_iter_next()

[Christoph Hellwig]

On Wed, Aug 19, 2026 at 05:07:05PM +0300, Tal Zussman wrote:
> I'm perfectly happy to do that. I just want the PageWriteback() gone
> and wanted to try to fix it properly rather than just delete the BUG()
> or throw in a page_folio(). But seems like "properly" in this case may
> be a deeper hole than I want to jump in to right now ;)

I'd vote for just removing it and unraveling that rest slowly.