[PATCH v3 0/2] netfs: fix the crash when unlocking the folio

[PATCH v3 0/2] netfs: fix the crash when unlocking the folio

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

V3:
- s/struct folio *folio/struct folio **folio/

V2:
- Add error_unlocked lable and rename error lable to error_locked.


kernel: page:00000000c9746ff1 refcount:2 mapcount:0 mapping:00000000dc2785bb index:0x1 pfn:0x141afc
kernel: memcg:ffff88810f766000
kernel: aops:ceph_aops [ceph] ino:100000005e7 dentry name:"postgresql-Fri.log" 
kernel: flags: 0x5ffc000000201c(uptodate|dirty|lru|private|node=0|zone=2|lastcpupid=0x7ff)
kernel: raw: 005ffc000000201c ffffea000a9eeb48 ffffea00060ade48 ffff888193ed8228
kernel: raw: 0000000000000001 ffff88810cc96500 00000002ffffffff ffff88810f766000
kernel: page dumped because: VM_BUG_ON_FOLIO(!folio_test_locked(folio))
kernel: ------------[ cut here ]------------
kernel: kernel BUG at mm/filemap.c:1559!
kernel: invalid opcode: 0000 [#1] PREEMPT SMP PTI
kernel: CPU: 4 PID: 131697 Comm: postmaster Tainted: G S                5.19.0-rc2-ceph-g822a4c74e05d #1
kernel: Hardware name: Supermicro SYS-5018R-WR/X10SRW-F, BIOS 2.0 12/17/2015
kernel: RIP: 0010:folio_unlock+0x26/0x30
kernel: Code: 00 0f 1f 00 0f 1f 44 00 00 48 8b 07 a8 01 74 0e f0 80 27 fe 78 01 c3 31 f6 e9 d6 fe ff ff 48 c7 c6 c0 81 37 82 e8 aa 64 04 00 <0f> 0b 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 48 8b 87 b8 01 00 00
kernel: RSP: 0018:ffffc90004377bc8 EFLAGS: 00010246
kernel: RAX: 000000000000003f RBX: ffff888193ed8228 RCX: 0000000000000001
kernel: RDX: 0000000000000000 RSI: ffffffff823a3569 RDI: 00000000ffffffff
kernel: RBP: ffffffff828a0058 R08: 0000000000000001 R09: 0000000000000001
kernel: R10: 000000007c6b0fd2 R11: 0000000000000034 R12: 0000000000000001
kernel: R13: 00000000fffffe00 R14: ffffea000506bf00 R15: ffff888193ed8000
kernel: FS:  00007f4993626340(0000) GS:ffff88885fd00000(0000) knlGS:0000000000000000
kernel: CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
kernel: CR2: 0000555789ee8000 CR3: 000000017a52a006 CR4: 00000000001706e0
kernel: Call Trace:
kernel: <TASK>
kernel: netfs_write_begin+0x130/0x950 [netfs]
kernel: ceph_write_begin+0x46/0xd0 [ceph]
kernel: generic_perform_write+0xef/0x200
kernel: ? file_update_time+0xd4/0x110
kernel: ceph_write_iter+0xb01/0xcd0 [ceph]
kernel: ? lock_is_held_type+0xe3/0x140
kernel: ? new_sync_write+0x106/0x180
kernel: new_sync_write+0x106/0x180
kernel: vfs_write+0x29a/0x3a0
kernel: ksys_write+0x5c/0xd0
kernel: do_syscall_64+0x34/0x80
kernel: entry_SYSCALL_64_after_hwframe+0x46/0xb0
kernel: RIP: 0033:0x7f49903205c8
kernel: Code: 89 02 48 c7 c0 ff ff ff ff eb b3 0f 1f 80 00 00 00 00 f3 0f 1e fa 48 8d 05 d5 3f 2a 00 8b 00 85 c0 75 17 b8 01 00 00 00 0f 05 <48> 3d 00 f0 ff ff 77 58 c3 0f 1f 80 00 00 00 00 41 54 49 89 d4 55
kernel: RSP: 002b:00007fff104bd178 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
kernel: RAX: ffffffffffffffda RBX: 0000000000000048 RCX: 00007f49903205c8
kernel: RDX: 0000000000000048 RSI: 000055944d3c1ea0 RDI: 000000000000000b
kernel: RBP: 000055944d3c1ea0 R08: 000055944d3963d0 R09: 00007fff1055b080
kernel: R10: 0000000000000000 R11: 0000000000000246 R12: 000055944d3962f0
kernel: R13: 0000000000000048 R14: 00007f49905bb880 R15: 0000000000000048
kernel: </TASK>


Xiubo Li (2):
  netfs: do not unlock and put the folio twice
  afs: unlock the folio when vnode is marked deleted

 fs/afs/file.c            | 10 ++++++++--
 fs/ceph/addr.c           | 11 ++++++-----
 fs/netfs/buffered_read.c | 18 ++++++++++--------
 include/linux/netfs.h    |  2 +-
 4 files changed, 25 insertions(+), 16 deletions(-)

-- 
2.36.0.rc1

[PATCH v3 1/2] netfs: do not unlock and put the folio twice

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

check_write_begin() will unlock and put the folio when return
non-zero. So we should avoid unlocking and putting it twice in
netfs layer.

At the same time pass a **folio to check_write_begin(), in which
the folio should be cleared after being put.

URL: https://tracker.ceph.com/issues/56423
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/afs/file.c            |  2 +-
 fs/ceph/addr.c           | 11 ++++++-----
 fs/netfs/buffered_read.c | 18 ++++++++++--------
 include/linux/netfs.h    |  2 +-
 4 files changed, 18 insertions(+), 15 deletions(-)

diff --git a/fs/afs/file.c b/fs/afs/file.c
index 42118a4f3383..afacce797fb9 100644
--- a/fs/afs/file.c
+++ b/fs/afs/file.c
@@ -375,7 +375,7 @@ static int afs_begin_cache_operation(struct netfs_io_request *rreq)
 }
 
 static int afs_check_write_begin(struct file *file, loff_t pos, unsigned len,
-				 struct folio *folio, void **_fsdata)
+				 struct folio **folio, void **_fsdata)
 {
 	struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
 
diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 8095fc47230e..cf909b08907f 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -63,7 +63,7 @@
 	 (CONGESTION_ON_THRESH(congestion_kb) >> 2))
 
 static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned int len,
-					struct folio *folio, void **_fsdata);
+					struct folio **folio, void **_fsdata);
 
 static inline struct ceph_snap_context *page_snap_context(struct page *page)
 {
@@ -1280,18 +1280,19 @@ ceph_find_incompatible(struct page *page)
 }
 
 static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned int len,
-					struct folio *folio, void **_fsdata)
+					struct folio **folio, void **_fsdata)
 {
 	struct inode *inode = file_inode(file);
 	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct ceph_snap_context *snapc;
 
-	snapc = ceph_find_incompatible(folio_page(folio, 0));
+	snapc = ceph_find_incompatible(folio_page(*folio, 0));
 	if (snapc) {
 		int r;
 
-		folio_unlock(folio);
-		folio_put(folio);
+		folio_unlock(*folio);
+		folio_put(*folio);
+		*folio = NULL;
 		if (IS_ERR(snapc))
 			return PTR_ERR(snapc);
 
diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index 42f892c5712e..308c2ad4da8e 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -319,8 +319,8 @@ static bool netfs_skip_folio_read(struct folio *folio, loff_t pos, size_t len,
  * conflicting writes once the folio is grabbed and locked.  It is passed a
  * pointer to the fsdata cookie that gets returned to the VM to be passed to
  * write_end.  It is permitted to sleep.  It should return 0 if the request
- * should go ahead; unlock the folio and return -EAGAIN to cause the folio to
- * be regot; or return an error.
+ * should go ahead; otherwise unlock, put and clear the folio and then return
+ * an error, -EAGAIN will cause the folio to be regot.
  *
  * The calling netfs must initialise a netfs context contiguous to the vfs
  * inode before calling this.
@@ -348,13 +348,14 @@ int netfs_write_begin(struct netfs_inode *ctx,
 
 	if (ctx->ops->check_write_begin) {
 		/* Allow the netfs (eg. ceph) to flush conflicts. */
-		ret = ctx->ops->check_write_begin(file, pos, len, folio, _fsdata);
+		ret = ctx->ops->check_write_begin(file, pos, len, &folio, _fsdata);
 		if (ret < 0) {
 			trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
 			if (ret == -EAGAIN)
 				goto retry;
-			goto error;
+			goto error_unlocked;
 		}
+		BUG_ON(!folio);
 	}
 
 	if (folio_test_uptodate(folio))
@@ -375,7 +376,7 @@ int netfs_write_begin(struct netfs_inode *ctx,
 				   NETFS_READ_FOR_WRITE);
 	if (IS_ERR(rreq)) {
 		ret = PTR_ERR(rreq);
-		goto error;
+		goto error_locked;
 	}
 	rreq->no_unlock_folio	= folio_index(folio);
 	__set_bit(NETFS_RREQ_NO_UNLOCK_FOLIO, &rreq->flags);
@@ -402,12 +403,12 @@ int netfs_write_begin(struct netfs_inode *ctx,
 
 	ret = netfs_begin_read(rreq, true);
 	if (ret < 0)
-		goto error;
+		goto error_locked;
 
 have_folio:
 	ret = folio_wait_fscache_killable(folio);
 	if (ret < 0)
-		goto error;
+		goto error_locked;
 have_folio_no_wait:
 	*_folio = folio;
 	_leave(" = 0");
@@ -415,9 +416,10 @@ int netfs_write_begin(struct netfs_inode *ctx,
 
 error_put:
 	netfs_put_request(rreq, false, netfs_rreq_trace_put_failed);
-error:
+error_locked:
 	folio_unlock(folio);
 	folio_put(folio);
+error_unlocked:
 	_leave(" = %d", ret);
 	return ret;
 }
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 1773e5df8e65..88c4eb85c6f7 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -214,7 +214,7 @@ struct netfs_request_ops {
 	void (*issue_read)(struct netfs_io_subrequest *subreq);
 	bool (*is_still_valid)(struct netfs_io_request *rreq);
 	int (*check_write_begin)(struct file *file, loff_t pos, unsigned len,
-				 struct folio *folio, void **_fsdata);
+				 struct folio **folio, void **_fsdata);
 	void (*done)(struct netfs_io_request *rreq);
 };
 
-- 
2.36.0.rc1

[PATCH v3 2/2] afs: unlock the folio when vnode is marked deleted

[xiubli]

From: Xiubo Li <xiubli@redhat.com>

The check_write_begin() should unlock the folio if return non-zero,
otherwise locked.

URL: https://tracker.ceph.com/issues/56423
Signed-off-by: Xiubo Li <xiubli@redhat.com>
---
 fs/afs/file.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/afs/file.c b/fs/afs/file.c
index afacce797fb9..b23e7b5a48ad 100644
--- a/fs/afs/file.c
+++ b/fs/afs/file.c
@@ -379,7 +379,13 @@ static int afs_check_write_begin(struct file *file, loff_t pos, unsigned len,
 {
 	struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
 
-	return test_bit(AFS_VNODE_DELETED, &vnode->flags) ? -ESTALE : 0;
+	if (test_bit(AFS_VNODE_DELETED, &vnode->flags)) {
+		folio_unlock(*folio);
+		folio_put(*folio);
+		return -ESTALE;
+	}
+
+	return 0;
 }
 
 static void afs_free_request(struct netfs_io_request *rreq)
-- 
2.36.0.rc1

Re: [PATCH v3 2/2] afs: unlock the folio when vnode is marked deleted

[Jeff Layton]

On Thu, 2022-07-07 at 12:51 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> The check_write_begin() should unlock the folio if return non-zero,
> otherwise locked.
> 
> URL: https://tracker.ceph.com/issues/56423
> Signed-off-by: Xiubo Li <xiubli@redhat.com>
> ---
>  fs/afs/file.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/afs/file.c b/fs/afs/file.c
> index afacce797fb9..b23e7b5a48ad 100644
> --- a/fs/afs/file.c
> +++ b/fs/afs/file.c
> @@ -379,7 +379,13 @@ static int afs_check_write_begin(struct file *file, loff_t pos, unsigned len,
>  {
>  	struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
>  
> -	return test_bit(AFS_VNODE_DELETED, &vnode->flags) ? -ESTALE : 0;
> +	if (test_bit(AFS_VNODE_DELETED, &vnode->flags)) {
> +		folio_unlock(*folio);
> +		folio_put(*folio);

Don't you also need this?

	*folio = NULL;

> +		return -ESTALE;
> +	}
> +
> +	return 0;
>  }
>  
>  static void afs_free_request(struct netfs_io_request *rreq)

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 1/2] netfs: do not unlock and put the folio twice

[David Howells]

xiubli@redhat.com wrote:

> URL: https://tracker.ceph.com/issues/56423

I think that should be "Link:".

David

Re: [PATCH v3 2/2] afs: unlock the folio when vnode is marked deleted

[David Howells]

Jeff Layton <jlayton@kernel.org> wrote:

> > +		folio_unlock(*folio);
> > +		folio_put(*folio);
> 
> Don't you also need this?
> 
> 	*folio = NULL;

It shouldn't need any of those three lines.

David

Re: [PATCH v3 1/2] netfs: do not unlock and put the folio twice

[David Howells]

xiubli@redhat.com wrote:

> -error:
> +error_locked:
>  	folio_unlock(folio);
>  	folio_put(folio);
> +error_unlocked:

I would do:

	error:
		if (folio) {
			folio_unlock(folio);
			folio_put(folio);
		}

David

[PATCH v4] netfs: do not unlock and put the folio twice

[David Howells]

Here's my take on this.  I've made the error: path handle folio == NULL, so
you don't need to split that error case.  I've also changed
->check_write_begin() so that it returns 0, not -EAGAIN, if we drop the folio;
the process is retried then if the folio pointer got cleared.

As a result, you don't have to discard the page if you want to return an error
and thus don't need the additional afs patch

David
---
commit 8489c89f6a186272593ab5e3fffbd47ea21185b7
Author: Xiubo Li <xiubli@redhat.com>
Date:   Thu Jul 7 12:51:11 2022 +0800

    netfs: do not unlock and put the folio twice
    
    check_write_begin() will unlock and put the folio when return
    non-zero.  So we should avoid unlocking and putting it twice in
    netfs layer.
    
    Change the way ->check_write_begin() works in the following two ways:
    
     (1) Pass it a pointer to the folio pointer, allowing it to unlock and put
         the folio prior to doing the stuff it wants to do, provided it clears
         the folio pointer.
    
     (2) Change the return values such that 0 with folio pointer set means
         continue, 0 with folio pointer cleared means re-get and all error
         codes indicating an error (no special treatment for -EAGAIN).
    
    Link: https://tracker.ceph.com/issues/56423
    Link: https://lore.kernel.org/r/20220707045112.10177-2-xiubli@redhat.com/
    Signed-off-by: Xiubo Li <xiubli@redhat.com>
    Co-developed-by: David Howells <dhowells@redhat.com>
    Signed-off-by: David Howells <dhowells@redhat.com>

diff --git a/Documentation/filesystems/netfs_library.rst b/Documentation/filesystems/netfs_library.rst
index 4d19b19bcc08..89085e1c22db 100644
--- a/Documentation/filesystems/netfs_library.rst
+++ b/Documentation/filesystems/netfs_library.rst
@@ -301,7 +301,7 @@ through which it can issue requests and negotiate::
 		void (*issue_read)(struct netfs_io_subrequest *subreq);
 		bool (*is_still_valid)(struct netfs_io_request *rreq);
 		int (*check_write_begin)(struct file *file, loff_t pos, unsigned len,
-					 struct folio *folio, void **_fsdata);
+					 struct folio **_folio, void **_fsdata);
 		void (*done)(struct netfs_io_request *rreq);
 	};
 
@@ -381,8 +381,10 @@ The operations are as follows:
    allocated/grabbed the folio to be modified to allow the filesystem to flush
    conflicting state before allowing it to be modified.
 
-   It should return 0 if everything is now fine, -EAGAIN if the folio should be
-   regrabbed and any other error code to abort the operation.
+   It may unlock and discard the folio it was given and set the caller's folio
+   pointer to NULL.  It should return 0 if everything is now fine (*_folio
+   left set) or the op should be retried (*_folio cleared) and any other error
+   code to abort the operation.
 
  * ``done``
 
diff --git a/fs/afs/file.c b/fs/afs/file.c
index 42118a4f3383..afacce797fb9 100644
--- a/fs/afs/file.c
+++ b/fs/afs/file.c
@@ -375,7 +375,7 @@ static int afs_begin_cache_operation(struct netfs_io_request *rreq)
 }
 
 static int afs_check_write_begin(struct file *file, loff_t pos, unsigned len,
-				 struct folio *folio, void **_fsdata)
+				 struct folio **folio, void **_fsdata)
 {
 	struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
 
diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 6dee88815491..ab070a24ca23 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -63,7 +63,7 @@
 	 (CONGESTION_ON_THRESH(congestion_kb) >> 2))
 
 static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned int len,
-					struct folio *folio, void **_fsdata);
+					struct folio **folio, void **_fsdata);
 
 static inline struct ceph_snap_context *page_snap_context(struct page *page)
 {
@@ -1288,18 +1288,19 @@ ceph_find_incompatible(struct page *page)
 }
 
 static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned int len,
-					struct folio *folio, void **_fsdata)
+					struct folio **folio, void **_fsdata)
 {
 	struct inode *inode = file_inode(file);
 	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct ceph_snap_context *snapc;
 
-	snapc = ceph_find_incompatible(folio_page(folio, 0));
+	snapc = ceph_find_incompatible(folio_page(*folio, 0));
 	if (snapc) {
 		int r;
 
-		folio_unlock(folio);
-		folio_put(folio);
+		folio_unlock(*folio);
+		folio_put(*folio);
+		*folio = NULL;
 		if (IS_ERR(snapc))
 			return PTR_ERR(snapc);
 
diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
index 42f892c5712e..69bbf1c25cf4 100644
--- a/fs/netfs/buffered_read.c
+++ b/fs/netfs/buffered_read.c
@@ -319,8 +319,9 @@ static bool netfs_skip_folio_read(struct folio *folio, loff_t pos, size_t len,
  * conflicting writes once the folio is grabbed and locked.  It is passed a
  * pointer to the fsdata cookie that gets returned to the VM to be passed to
  * write_end.  It is permitted to sleep.  It should return 0 if the request
- * should go ahead; unlock the folio and return -EAGAIN to cause the folio to
- * be regot; or return an error.
+ * should go ahead or it may return an error.  It may also unlock and put the
+ * folio, provided it sets *_folio to NULL, in which case a return of 0 will
+ * cause the folio to be re-got and the process to be retried.
  *
  * The calling netfs must initialise a netfs context contiguous to the vfs
  * inode before calling this.
@@ -348,13 +349,13 @@ int netfs_write_begin(struct netfs_inode *ctx,
 
 	if (ctx->ops->check_write_begin) {
 		/* Allow the netfs (eg. ceph) to flush conflicts. */
-		ret = ctx->ops->check_write_begin(file, pos, len, folio, _fsdata);
+		ret = ctx->ops->check_write_begin(file, pos, len, &folio, _fsdata);
 		if (ret < 0) {
 			trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
-			if (ret == -EAGAIN)
-				goto retry;
 			goto error;
 		}
+		if (!folio)
+			goto retry;
 	}
 
 	if (folio_test_uptodate(folio))
@@ -416,8 +417,10 @@ int netfs_write_begin(struct netfs_inode *ctx,
 error_put:
 	netfs_put_request(rreq, false, netfs_rreq_trace_put_failed);
 error:
-	folio_unlock(folio);
-	folio_put(folio);
+	if (folio) {
+		folio_unlock(folio);
+		folio_put(folio);
+	}
 	_leave(" = %d", ret);
 	return ret;
 }
diff --git a/include/linux/netfs.h b/include/linux/netfs.h
index 1773e5df8e65..6ab5d56dac74 100644
--- a/include/linux/netfs.h
+++ b/include/linux/netfs.h
@@ -214,7 +214,7 @@ struct netfs_request_ops {
 	void (*issue_read)(struct netfs_io_subrequest *subreq);
 	bool (*is_still_valid)(struct netfs_io_request *rreq);
 	int (*check_write_begin)(struct file *file, loff_t pos, unsigned len,
-				 struct folio *folio, void **_fsdata);
+				 struct folio **_folio, void **_fsdata);
 	void (*done)(struct netfs_io_request *rreq);
 };
 

Re: [PATCH v4] netfs: do not unlock and put the folio twice

[Matthew Wilcox]

On Thu, Jul 07, 2022 at 02:21:45PM +0100, David Howells wrote:
> -					 struct folio *folio, void **_fsdata);
> +					 struct folio **_folio, void **_fsdata);

The usual convention is that _foo means "Don't touch".  This should
probably be named "foliop" (ie pointer to a thing that would normally
be called folio).

Re: [PATCH v3 1/2] netfs: do not unlock and put the folio twice

[Xiubo Li]

On 7/7/22 8:32 PM, David Howells wrote:
> xiubli@redhat.com wrote:
>
>> URL: https://tracker.ceph.com/issues/56423
> I think that should be "Link:".

Okay, in ceph tree we are using the "URL:".

-- Xiubo

>
> David
>

Re: [PATCH v4] netfs: do not unlock and put the folio twice

[Xiubo Li]

On 7/7/22 9:21 PM, David Howells wrote:
> Here's my take on this.  I've made the error: path handle folio == NULL, so
> you don't need to split that error case.  I've also changed
> ->check_write_begin() so that it returns 0, not -EAGAIN, if we drop the folio;
> the process is retried then if the folio pointer got cleared.
>
> As a result, you don't have to discard the page if you want to return an error
> and thus don't need the additional afs patch
>
> David
> ---
> commit 8489c89f6a186272593ab5e3fffbd47ea21185b7
> Author: Xiubo Li <xiubli@redhat.com>
> Date:   Thu Jul 7 12:51:11 2022 +0800
>
>      netfs: do not unlock and put the folio twice
>      
>      check_write_begin() will unlock and put the folio when return
>      non-zero.  So we should avoid unlocking and putting it twice in
>      netfs layer.
>      
>      Change the way ->check_write_begin() works in the following two ways:
>      
>       (1) Pass it a pointer to the folio pointer, allowing it to unlock and put
>           the folio prior to doing the stuff it wants to do, provided it clears
>           the folio pointer.
>      
>       (2) Change the return values such that 0 with folio pointer set means
>           continue, 0 with folio pointer cleared means re-get and all error
>           codes indicating an error (no special treatment for -EAGAIN).
>      
>      Link: https://tracker.ceph.com/issues/56423
>      Link: https://lore.kernel.org/r/20220707045112.10177-2-xiubli@redhat.com/
>      Signed-off-by: Xiubo Li <xiubli@redhat.com>
>      Co-developed-by: David Howells <dhowells@redhat.com>
>      Signed-off-by: David Howells <dhowells@redhat.com>
>
> diff --git a/Documentation/filesystems/netfs_library.rst b/Documentation/filesystems/netfs_library.rst
> index 4d19b19bcc08..89085e1c22db 100644
> --- a/Documentation/filesystems/netfs_library.rst
> +++ b/Documentation/filesystems/netfs_library.rst
> @@ -301,7 +301,7 @@ through which it can issue requests and negotiate::
>   		void (*issue_read)(struct netfs_io_subrequest *subreq);
>   		bool (*is_still_valid)(struct netfs_io_request *rreq);
>   		int (*check_write_begin)(struct file *file, loff_t pos, unsigned len,
> -					 struct folio *folio, void **_fsdata);
> +					 struct folio **_folio, void **_fsdata);
>   		void (*done)(struct netfs_io_request *rreq);
>   	};
>   
> @@ -381,8 +381,10 @@ The operations are as follows:
>      allocated/grabbed the folio to be modified to allow the filesystem to flush
>      conflicting state before allowing it to be modified.
>   
> -   It should return 0 if everything is now fine, -EAGAIN if the folio should be
> -   regrabbed and any other error code to abort the operation.
> +   It may unlock and discard the folio it was given and set the caller's folio
> +   pointer to NULL.  It should return 0 if everything is now fine (*_folio
> +   left set) or the op should be retried (*_folio cleared) and any other error
> +   code to abort the operation.
>   
>    * ``done``
>   
> diff --git a/fs/afs/file.c b/fs/afs/file.c
> index 42118a4f3383..afacce797fb9 100644
> --- a/fs/afs/file.c
> +++ b/fs/afs/file.c
> @@ -375,7 +375,7 @@ static int afs_begin_cache_operation(struct netfs_io_request *rreq)
>   }
>   
>   static int afs_check_write_begin(struct file *file, loff_t pos, unsigned len,
> -				 struct folio *folio, void **_fsdata)
> +				 struct folio **folio, void **_fsdata)
>   {
>   	struct afs_vnode *vnode = AFS_FS_I(file_inode(file));
>   
> diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
> index 6dee88815491..ab070a24ca23 100644
> --- a/fs/ceph/addr.c
> +++ b/fs/ceph/addr.c
> @@ -63,7 +63,7 @@
>   	 (CONGESTION_ON_THRESH(congestion_kb) >> 2))
>   
>   static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned int len,
> -					struct folio *folio, void **_fsdata);
> +					struct folio **folio, void **_fsdata);
>   
>   static inline struct ceph_snap_context *page_snap_context(struct page *page)
>   {
> @@ -1288,18 +1288,19 @@ ceph_find_incompatible(struct page *page)
>   }
>   
>   static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned int len,
> -					struct folio *folio, void **_fsdata)
> +					struct folio **folio, void **_fsdata)
>   {
>   	struct inode *inode = file_inode(file);
>   	struct ceph_inode_info *ci = ceph_inode(inode);
>   	struct ceph_snap_context *snapc;
>   
> -	snapc = ceph_find_incompatible(folio_page(folio, 0));
> +	snapc = ceph_find_incompatible(folio_page(*folio, 0));
>   	if (snapc) {
>   		int r;
>   
> -		folio_unlock(folio);
> -		folio_put(folio);
> +		folio_unlock(*folio);
> +		folio_put(*folio);
> +		*folio = NULL;
>   		if (IS_ERR(snapc))
>   			return PTR_ERR(snapc);
>   
> diff --git a/fs/netfs/buffered_read.c b/fs/netfs/buffered_read.c
> index 42f892c5712e..69bbf1c25cf4 100644
> --- a/fs/netfs/buffered_read.c
> +++ b/fs/netfs/buffered_read.c
> @@ -319,8 +319,9 @@ static bool netfs_skip_folio_read(struct folio *folio, loff_t pos, size_t len,
>    * conflicting writes once the folio is grabbed and locked.  It is passed a
>    * pointer to the fsdata cookie that gets returned to the VM to be passed to
>    * write_end.  It is permitted to sleep.  It should return 0 if the request
> - * should go ahead; unlock the folio and return -EAGAIN to cause the folio to
> - * be regot; or return an error.
> + * should go ahead or it may return an error.  It may also unlock and put the
> + * folio, provided it sets *_folio to NULL, in which case a return of 0 will
> + * cause the folio to be re-got and the process to be retried.
>    *
>    * The calling netfs must initialise a netfs context contiguous to the vfs
>    * inode before calling this.
> @@ -348,13 +349,13 @@ int netfs_write_begin(struct netfs_inode *ctx,
>   
>   	if (ctx->ops->check_write_begin) {
>   		/* Allow the netfs (eg. ceph) to flush conflicts. */
> -		ret = ctx->ops->check_write_begin(file, pos, len, folio, _fsdata);
> +		ret = ctx->ops->check_write_begin(file, pos, len, &folio, _fsdata);
>   		if (ret < 0) {
>   			trace_netfs_failure(NULL, NULL, ret, netfs_fail_check_write_begin);
> -			if (ret == -EAGAIN)
> -				goto retry;
>   			goto error;
>   		}
> +		if (!folio)
> +			goto retry;
>   	}
>   
>   	if (folio_test_uptodate(folio))
> @@ -416,8 +417,10 @@ int netfs_write_begin(struct netfs_inode *ctx,
>   error_put:
>   	netfs_put_request(rreq, false, netfs_rreq_trace_put_failed);
>   error:
> -	folio_unlock(folio);
> -	folio_put(folio);
> +	if (folio) {
> +		folio_unlock(folio);
> +		folio_put(folio);
> +	}
>   	_leave(" = %d", ret);
>   	return ret;
>   }
> diff --git a/include/linux/netfs.h b/include/linux/netfs.h
> index 1773e5df8e65..6ab5d56dac74 100644
> --- a/include/linux/netfs.h
> +++ b/include/linux/netfs.h
> @@ -214,7 +214,7 @@ struct netfs_request_ops {
>   	void (*issue_read)(struct netfs_io_subrequest *subreq);
>   	bool (*is_still_valid)(struct netfs_io_request *rreq);
>   	int (*check_write_begin)(struct file *file, loff_t pos, unsigned len,
> -				 struct folio *folio, void **_fsdata);
> +				 struct folio **_folio, void **_fsdata);
>   	void (*done)(struct netfs_io_request *rreq);
>   };
>   
>
This version looks good to me.

Thanks David!

Re: [PATCH v4] netfs: do not unlock and put the folio twice

[Xiubo Li]

On 7/7/22 9:26 PM, Matthew Wilcox wrote:
> On Thu, Jul 07, 2022 at 02:21:45PM +0100, David Howells wrote:
>> -					 struct folio *folio, void **_fsdata);
>> +					 struct folio **_folio, void **_fsdata);
> The usual convention is that _foo means "Don't touch".  This should
> probably be named "foliop" (ie pointer to a thing that would normally
> be called folio).
>
This looks good to me.

I will send out the V5 by fixing this and will merge this patch via the 
ceph tree as discussed with David in the IRC and will cc the stable at 
the same time.

Thanks!

-- Xiubo