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

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

[slava]

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.

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

[Fan Ni]

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

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

[David Howells]

Hi Fan,

My aim is to get rid of all page/folio handling from the main part of the
filesystem entirely and use netfslib instead.  See:

	https://lore.kernel.org/linux-fsdevel/20250313233341.1675324-1-dhowells@redhat.com/T/#u

Now, this is a work in progress, but I think I have a decent shot at having it
ready for the next merge window after the one that should open in about a
week.

Note that there, struct ceph_snap_context is built around a netfs_group struct
and attachment to folios is handled by netfslib as much as possible.

My patches can be obtained here:

	https://web.git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=ceph-iter

David

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

[Fan Ni]

On Tue, Mar 18, 2025 at 10:42:05PM +0000, David Howells wrote:
> Hi Fan,
> 
> My aim is to get rid of all page/folio handling from the main part of the
> filesystem entirely and use netfslib instead.  See:
> 
> 	https://lore.kernel.org/linux-fsdevel/20250313233341.1675324-1-dhowells@redhat.com/T/#u
> 
> Now, this is a work in progress, but I think I have a decent shot at having it
> ready for the next merge window after the one that should open in about a
> week.
> 
> Note that there, struct ceph_snap_context is built around a netfs_group struct
> and attachment to folios is handled by netfslib as much as possible.
> 
> My patches can be obtained here:
> 
> 	https://web.git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?h=ceph-iter
> 
> David
> 
Hi David,

Thanks for your information. 
That is very useful information to me, since I am still slowly ramp-up mm work and lack
of the whole picture of mm development work. 

Just to make it more clear to me, so that means all &folio->page and like
will be taken care of with your patches for fs/, right? If so, I will skip fs
and try to work on other sub-system.

Fan

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

[slava]

On Tue, 2025-03-18 at 16:25 -0700, Fan Ni wrote:
> On Tue, Mar 18, 2025 at 10:42:05PM +0000, David Howells wrote:
> > Hi Fan,
> > 
> > My aim is to get rid of all page/folio handling from the main part
> > of the
> > filesystem entirely and use netfslib instead.  See:
> > 
> > 	
> > https://lore.kernel.org/linux-fsdevel/20250313233341.1675324-1-dho
> > wells@redhat.com/T/#u
> > 
> > Now, this is a work in progress, but I think I have a decent shot
> > at having it
> > ready for the next merge window after the one that should open in
> > about a
> > week.
> > 
> > Note that there, struct ceph_snap_context is built around a
> > netfs_group struct
> > and attachment to folios is handled by netfslib as much as
> > possible.
> > 
> > My patches can be obtained here:
> > 
> > 	
> > https://web.git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-
> > fs.git/log/?h=ceph-iter
> > 
> > David
> > 
> Hi David,
> 
> Thanks for your information. 
> That is very useful information to me, since I am still slowly ramp-
> up mm work and lack
> of the whole picture of mm development work. 
> 
> Just to make it more clear to me, so that means all &folio->page and
> like
> will be taken care of with your patches for fs/, right? If so, I will
> skip fs
> and try to work on other sub-system.
> 

As far as I can see, we still have a lot of work in fs/ and CephFS code
for switching from page to folio. :) So, you are really welcome to
contribute.

Thanks,
Slava.

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

[David Howells]

Fan Ni <nifan.cxl@gmail.com> wrote:

> That is very useful information to me, since I am still slowly ramp-up mm work and lack
> of the whole picture of mm development work. 
> 
> Just to make it more clear to me, so that means all &folio->page and like
> will be taken care of with your patches for fs/, right? If so, I will skip fs
> and try to work on other sub-system.

Yes-ish.  What you're trying to do might already have been taken care of by
Willy for the next merge window:

	https://web.git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/log/?h=vfs-6.15.ceph

I based my branch on this.

David