Re: [FUSE] notify_store usage: deadlocks with other read / write requests

[Miklos Szeredi <miklos@szeredi.hu>]

On Fri, Aug 27, 2021 at 05:31:18PM +0000, Teng Qin wrote:
> I am developing a file system that has underlying block size way larger than the number of pages VFS would request to the FUSE daemon (2MB / 4MB vs 32 pages = 128K).
> I currently cache the block data in user space, but it would be more ideal to have Kernel manage this with page cache, and save round-trips between VFS and FUSE daemon. So I was looking at use FUSE_NOTIFY_STORE to proactively offer the data to Kernel. However, I found that the notify store often deadlocks with user read requests.
> 
> For example, say the user process is doing sequential read from offset 0.
> Kernel requests a 128K read to FUSE daemon and I fetch the 2MB block from underlying storage. After replying the read request, I would like to offer the rest of the 1920K data to Kernel from offset 128K. However, at this point Kernel most likely alraedy started the next read request also at offset 128K, and have those page locked:
> 
>   wait_on_page_locked_killable
>   generic_file_buffered_read
>   generic_file_read_iter
> 
> On the other hand, the notify store is also waiting on locking those pages:
> 
>   __lock_page
>   __find_lock_page
>   find_or_create_page
>   fuse_notify_store
> 
> This normally deadlocks the FUSE daemon.
> 
> The notify store is a pretty old feature so I'm not sure if this is really an issue or I'm using it wrong. I would be very grateful if anyone could help me with some insights on how this is intended to be used. On the other hand, I was thinking maybe we could support an async notify store requests. When the Kernel moduels gets the requests, if it can not acquire lock on the relevant pages, it could just store the user provided data in dis-attached page structs, add them to a background requetss, and try later. If people are OK with such ideas, I would be more than happy to try with an implementation.

Hi,

Simplest solution is to just skip locked pages in NOTIFY_STORE.  Can you try the
attached patch (untested)?

Thanks,
Miklos

---
 fs/fuse/dev.c             |   16 +++++++++++++---
 include/uapi/linux/fuse.h |    9 ++++++++-
 2 files changed, 21 insertions(+), 4 deletions(-)

--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1562,6 +1562,7 @@ static int fuse_notify_store(struct fuse
 	unsigned int num;
 	loff_t file_size;
 	loff_t end;
+	bool nowait;
 
 	err = -EINVAL;
 	if (size < sizeof(outarg))
@@ -1576,6 +1577,7 @@ static int fuse_notify_store(struct fuse
 		goto out_finish;
 
 	nodeid = outarg.nodeid;
+	nowait = outarg.flags & FUSE_NOTIFY_STORE_NOWAIT;
 
 	down_read(&fc->killsb);
 
@@ -1598,12 +1600,19 @@ static int fuse_notify_store(struct fuse
 	while (num) {
 		struct page *page;
 		unsigned int this_num;
+		int fgp_flags = FGP_LOCK | FGP_ACCESSED | FGP_CREAT;
+
+		if (nowait)
+			fgp_flags |= FGP_NOWAIT;
 
 		err = -ENOMEM;
-		page = find_or_create_page(mapping, index,
-					   mapping_gfp_mask(mapping));
-		if (!page)
+		page = pagecache_get_page(mapping, index, fgp_flags,
+					  mapping_gfp_mask(mapping));
+		if (!page) {
+			if (nowait)
+				goto skip;
 			goto out_iput;
+		}
 
 		this_num = min_t(unsigned, num, PAGE_SIZE - offset);
 		err = fuse_copy_page(cs, &page, offset, this_num, 0);
@@ -1616,6 +1625,7 @@ static int fuse_notify_store(struct fuse
 		if (err)
 			goto out_iput;
 
+skip:
 		num -= this_num;
 		offset = 0;
 		index++;
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -464,6 +464,13 @@ struct fuse_file_lock {
  */
 #define FUSE_SETXATTR_ACL_KILL_SGID	(1 << 0)
 
+
+/*
+ * notify_store flags
+ * FUSE_NOTIFY_STORE_NOWAIT: skip locked pages
+ */
+#define FUSE_NOTIFY_STORE_NOWAIT	(1 << 0)
+
 enum fuse_opcode {
 	FUSE_LOOKUP		= 1,
 	FUSE_FORGET		= 2,  /* no reply */
@@ -899,7 +906,7 @@ struct fuse_notify_store_out {
 	uint64_t	nodeid;
 	uint64_t	offset;
 	uint32_t	size;
-	uint32_t	padding;
+	uint32_t	flags;
 };
 
 struct fuse_notify_retrieve_out {