Re: Question about code in fs/ceph/addr.c

[Fan Ni <nifan.cxl@gmail.com>]

On Tue, Mar 18, 2025 at 12:00:52PM -0700, slava@dubeyko.com wrote:
> Hi Fan,
> 
> + CC David Howells <dhowells@redhat.com>
> + CC ceph-devel@vger.kernel.org
> 
> On Tue, 2025-03-18 at 11:30 -0700, Fan Ni wrote:
> > Hi Viacheslav,
> > 
> > This is Fan Ni. Recently I started to work on some mm work. One thing
> > that I am working on is to reduce the use of &folio->page. When I
> > check
> > the fs/ceph code, I find some code that may be good candidate for the
> > work to be done.
> > 
> > I see you sent some patches to add ceph_submit_write(), since the
> > change
> > I am planning to do is closely related to it, so I reach out to you
> > to
> > see if you have some input for me.
> > 
> > Based on my reading of the code, it seems ceph_wbc->pages[i] will
> > always be the head page of the folio involved. I am thinking maybe we
> > can
> > keep folios instead of pages here, do you see any reason we should
> > not
> > use folios here and must be pages?
> > 
> 
> I believe we need to switch from pages to folios in CephFS code. But it
> is painful modification. We need to be really careful in this.
> 
> As far as I know, David Howells is making significant modification
> namely in this direction. I think you need to synchronize the
> implementation activity with him. I'd love to be involved but,
> currently, I am focused on fixing other issues in CephFS code. :)
> 
> Thanks,
> Slava.
> 

Thanks Slava,

Let me add more context here. 
I have a patch as below, which only handle case 1 as mentioned in 
the commit log. The question I am asking here is related to the
handling of case 2 as the page passed to ceph_fscrypt_pagecache_page
is from ceph_wbc->pages[i]. After checking the code, I think it can
be folio instead of page.
My goal is to remove page_snap_context() and use folio_snap_context().

Fan


From 02b26cad4f5cb0b43047cf6e343ba5257f95c6ee Mon Sep 17 00:00:00 2001
From: Fan Ni <fan.ni@samsung.com>
Date: Mon, 17 Mar 2025 14:29:06 -0700
Subject: [PATCH] fs/ceph: Introduce folio_snap_context to avoid passing
 &folio->page

The functions that call page_snap_context() either passes in
1) &folio->page, which is the common case; or
2) ceph_fscrypt_pagecache_page(page), which only happens in
get_writepages_data_length().

We separate the handling of the call to page_snap_context for the two cases.
For the first case, we remove the use of &folio->page by introducing an
equivalent function folio_snap_context() which consumes struct folio directly.
For the second case, it involves more changes to convert, we leave it
unchanged for now.

Signed-off-by: Fan Ni <fan.ni@samsung.com>
---
 fs/ceph/addr.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 29be367905a1..aa0ad730059a 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -67,6 +67,13 @@
 static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned int len,
 					struct folio **foliop, void **_fsdata);
 
+static inline struct ceph_snap_context *folio_snap_context(struct folio *folio)
+{
+	if (folio_test_private(folio))
+		return (void *)folio->private;
+	return NULL;
+}
+
 static inline struct ceph_snap_context *page_snap_context(struct page *page)
 {
 	if (PagePrivate(page))
@@ -729,7 +736,7 @@ static int write_folio_nounlock(struct folio *folio,
 		return -EIO;
 
 	/* verify this is a writeable snap context */
-	snapc = page_snap_context(&folio->page);
+	snapc = folio_snap_context(folio);
 	if (!snapc) {
 		doutc(cl, "%llx.%llx folio %p not dirty?\n", ceph_vinop(inode),
 		      folio);
@@ -1140,7 +1147,7 @@ int ceph_check_page_before_write(struct address_space *mapping,
 	}
 
 	/* only if matching snap context */
-	pgsnapc = page_snap_context(&folio->page);
+	pgsnapc = folio_snap_context(folio);
 	if (pgsnapc != ceph_wbc->snapc) {
 		doutc(cl, "folio snapc %p %lld != oldest %p %lld\n",
 		      pgsnapc, pgsnapc->seq,
@@ -1586,7 +1593,7 @@ void ceph_wait_until_current_writes_complete(struct address_space *mapping,
 					     struct writeback_control *wbc,
 					     struct ceph_writeback_ctl *ceph_wbc)
 {
-	struct page *page;
+	struct folio *folio;
 	unsigned i, nr;
 
 	if (wbc->sync_mode != WB_SYNC_NONE &&
@@ -1601,10 +1608,10 @@ void ceph_wait_until_current_writes_complete(struct address_space *mapping,
 						     PAGECACHE_TAG_WRITEBACK,
 						     &ceph_wbc->fbatch))) {
 			for (i = 0; i < nr; i++) {
-				page = &ceph_wbc->fbatch.folios[i]->page;
-				if (page_snap_context(page) != ceph_wbc->snapc)
+				folio = ceph_wbc->fbatch.folios[i];
+				if (folio_snap_context(folio) != ceph_wbc->snapc)
 					continue;
-				wait_on_page_writeback(page);
+				folio_wait_writeback(folio);
 			}
 
 			folio_batch_release(&ceph_wbc->fbatch);
@@ -1793,7 +1800,7 @@ ceph_find_incompatible(struct folio *folio)
 
 		folio_wait_writeback(folio);
 
-		snapc = page_snap_context(&folio->page);
+		snapc = folio_snap_context(folio);
 		if (!snapc || snapc == ci->i_head_snapc)
 			break;
 
-- 
2.47.2