Re: [GIT PULL] fscache: I/O API modernisation and netfs helper library

[David Howells]

Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:

> ...
> IOW, I'm not against "wait_on_page_fscache()" as a function, but I
> *am* against the odd _mixing_ of things without a big explanation,
> where the code itself looks very odd and questionable.
> 
> And I think the "fscache" waiting functions should not be visible to
> any core VM or filesystem code - it should be limited explicitly to
> those filesystems that use fscache, and include that header file.

Okay...  How about the attached then?

I've also discarded the patch that just moves towards completely getting rid
of PG_fscache and adjusted the third patch that takes a ref on the page for
the duration to handle the change of names.

Speaking of the ref-taking patch, is the one I posted yesterday the sort of
thing you wanted for that?  I wonder if I should drop the ref in the unlock
function, though doing it afterwards does allow for the possibility of using a
pagevec to do mass-release.

> Wouldn't that make sense?

Well, that's the current principle, but I was wondering if the alias was
causing confusion.

David
---
commit c723f0232c9f8928b3b15786499637bda3121f41
Author: David Howells <dhowells@xxxxxxxxxx>
Date:   Wed Feb 10 10:53:02 2021 +0000

    netfs: Rename unlock_page_fscache() and move wait_on_page_fscache()
    
    Rename unlock_page_fscache() to unlock_page_private_2() and change the
    references to PG_fscache to PG_private_2 also.  This makes it look more
    generic and doesn't mix the terminology.
    
    Fix the kdoc comment on the above as the wake up mechanism doesn't wake up
    all the sleepers.  Note the example usage case for the function in
    conjunction with the cache also.
    
    Place unlock_page_fscache() as an alias in linux/netfs.h.
    
    Move wait_on_page_fscache() to linux/netfs.h.
    
    [v2: Implement suggestion by Linus to move the wait function into netfs.h]
    
    Reported-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
    Signed-off-by: David Howells <dhowells@xxxxxxxxxx>
    Tested-by: Jeff Layton <jlayton@xxxxxxxxxx>
    Link: https://lore.kernel.org/linux-fsdevel/1330473.1612974547@xxxxxxxxxxxxxxxxxxxxxx/
    Link: https://lore.kernel.org/linux-fsdevel/CAHk-=wjgA-74ddehziVk=XAEMTKswPu1Yw4uaro1R3ibs27ztw@xxxxxxxxxxxxxx/

diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 2ffdef1ded91..59c2623dc408 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -14,6 +14,7 @@
 
 #include <linux/workqueue.h>
 #include <linux/fs.h>
+#include <linux/pagemap.h>
 
 /*
  * Overload PG_private_2 to give us PG_fscache - this is used to indicate that
@@ -25,6 +26,35 @@
 #define TestSetPageFsCache(page)	TestSetPagePrivate2((page))
 #define TestClearPageFsCache(page)	TestClearPagePrivate2((page))
 
+/**
+ * unlock_page_fscache - Unlock a page that's locked with PG_fscache
+ * @page: The page
+ *
+ * Unlocks a page that's locked with PG_fscache and wakes up sleepers in
+ * wait_on_page_fscache().  This page bit is used by the netfs helpers when a
+ * netfs page is being written to a local disk cache, thereby allowing writes
+ * to the cache for the same page to be serialised.
+ */
+static inline void unlock_page_fscache(struct page *page)
+{
+	unlock_page_private_2(page);
+}
+
+/**
+ * wait_on_page_fscache - Wait for PG_fscache to be cleared on a page
+ * @page: The page
+ *
+ * Wait for the PG_fscache (PG_private_2) page bit to be removed from a page.
+ * This is, for example, used to handle a netfs page being written to a local
+ * disk cache, thereby allowing writes to the cache for the same page to be
+ * serialised.
+ */
+static inline void wait_on_page_fscache(struct page *page)
+{
+	if (PageFsCache(page))
+		wait_on_page_bit(compound_head(page), PG_fscache);
+}
+
 enum netfs_read_source {
 	NETFS_FILL_WITH_ZEROES,
 	NETFS_DOWNLOAD_FROM_SERVER,
diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 4935ad6171c1..d2786607d297 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -591,7 +591,7 @@ extern int __lock_page_async(struct page *page, struct wait_page_queue *wait);
 extern int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
 				unsigned int flags);
 extern void unlock_page(struct page *page);
-extern void unlock_page_fscache(struct page *page);
+extern void unlock_page_private_2(struct page *page);
 
 /*
  * Return true if the page was successfully locked
@@ -682,19 +682,6 @@ static inline int wait_on_page_locked_killable(struct page *page)
 	return wait_on_page_bit_killable(compound_head(page), PG_locked);
 }
 
-/**
- * wait_on_page_fscache - Wait for PG_fscache to be cleared on a page
- * @page: The page
- *
- * Wait for the fscache mark to be removed from a page, usually signifying the
- * completion of a write from that page to the cache.
- */
-static inline void wait_on_page_fscache(struct page *page)
-{
-	if (PagePrivate2(page))
-		wait_on_page_bit(compound_head(page), PG_fscache);
-}
-
 extern void put_and_wait_on_page_locked(struct page *page);
 
 void wait_on_page_writeback(struct page *page);
diff --git a/mm/filemap.c b/mm/filemap.c
index 91fcae006d64..7d321152d579 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1467,22 +1467,24 @@ void unlock_page(struct page *page)
 EXPORT_SYMBOL(unlock_page);
 
 /**
- * unlock_page_fscache - Unlock a page pinned with PG_fscache
+ * unlock_page_private_2 - Unlock a page that's locked with PG_private_2
  * @page: The page
  *
- * Unlocks the page and wakes up sleepers in wait_on_page_fscache().  Also
- * wakes those waiting for the lock and writeback bits because the wakeup
- * mechanism is shared.  But that's OK - those sleepers will just go back to
- * sleep.
+ * Unlocks a page that's locked with PG_private_2 and wakes up sleepers in
+ * wait_on_page_private_2().
+ *
+ * This is, for example, used when a netfs page is being written to a local
+ * disk cache, thereby allowing writes to the cache for the same page to be
+ * serialised.
  */
-void unlock_page_fscache(struct page *page)
+void unlock_page_private_2(struct page *page)
 {
 	page = compound_head(page);
 	VM_BUG_ON_PAGE(!PagePrivate2(page), page);
-	clear_bit_unlock(PG_fscache, &page->flags);
-	wake_up_page_bit(page, PG_fscache);
+	clear_bit_unlock(PG_private_2, &page->flags);
+	wake_up_page_bit(page, PG_private_2);
 }
-EXPORT_SYMBOL(unlock_page_fscache);
+EXPORT_SYMBOL(unlock_page_private_2);
 
 /**
  * end_page_writeback - end writeback against a page