Hi!
On Wed, Sep 08 2021, Miklos Szeredi wrote:
> On Wed, Sep 08, 2021 at 11:30:13AM +0200, Miklos Szeredi wrote:
>> 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)?
>
> And another version (data needs to be skipped as well).
Resurrecting an old thread, as I believe I'm able to reproduce this issue.
I'm inlining below my attempt to forward port the original patch to the
folios world. It seems to work as expected, but I'm not sure if it's
correct. For example, if we fail to get a folio and need to skip it,
'this_num' would need to be updated; but it's not clear if using PAGE_SIZE
in that case is correct. An alternative would be to simply return an
error to userpace immediately at that point.
Cheers,
--
Luís
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 27ccae63495d..712388d8a3bd 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -1630,6 +1630,7 @@ static int fuse_notify_store(struct fuse_conn *fc, unsigned int size,
unsigned int num;
loff_t file_size;
loff_t end;
+ int fgp_flags = FGP_LOCK | FGP_ACCESSED | FGP_CREAT;
err = -EINVAL;
if (size < sizeof(outarg))
@@ -1645,6 +1646,9 @@ static int fuse_notify_store(struct fuse_conn *fc, unsigned int size,
nodeid = outarg.nodeid;
+ if (outarg.flags & FUSE_NOTIFY_STORE_NOWAIT)
+ fgp_flags |= FGP_NOWAIT;
+
down_read(&fc->killsb);
err = -ENOENT;
@@ -1668,14 +1672,24 @@ static int fuse_notify_store(struct fuse_conn *fc, unsigned int size,
struct page *page;
unsigned int this_num;
- folio = filemap_grab_folio(mapping, index);
- err = PTR_ERR(folio);
- if (IS_ERR(folio))
- goto out_iput;
+ folio = __filemap_get_folio(mapping, index, fgp_flags,
+ mapping_gfp_mask(mapping));
+ err = PTR_ERR_OR_ZERO(folio);
+ if (err) {
+ if (!(outarg.flags & FUSE_NOTIFY_STORE_NOWAIT))
+ goto out_iput;
+ page = NULL;
+ this_num = min_t(unsigned, num, PAGE_SIZE - offset); /* XXX */
+ } else {
+ page = &folio->page;
+ this_num = min_t(unsigned, num, folio_size(folio) -
+ offset);
+ }
- page = &folio->page;
- this_num = min_t(unsigned, num, folio_size(folio) - offset);
err = fuse_copy_page(cs, &page, offset, this_num, 0);
+ if (!page)
+ goto skip;
+
if (!folio_test_uptodate(folio) && !err && offset == 0 &&
(this_num == folio_size(folio) || file_size == end)) {
folio_zero_segment(folio, this_num, folio_size(folio));
@@ -1683,7 +1697,7 @@ static int fuse_notify_store(struct fuse_conn *fc, unsigned int size,
}
folio_unlock(folio);
folio_put(folio);
-
+skip:
if (err)
goto out_iput;
diff --git a/include/uapi/linux/fuse.h b/include/uapi/linux/fuse.h
index e9e78292d107..59725f89340e 100644
--- a/include/uapi/linux/fuse.h
+++ b/include/uapi/linux/fuse.h
@@ -576,6 +576,12 @@ struct fuse_file_lock {
*/
#define FUSE_EXPIRE_ONLY (1 << 0)
+/**
+ * notify_store flags
+ * FUSE_NOTIFY_STORE_NOWAIT: skip locked pages
+ */
+#define FUSE_NOTIFY_STORE_NOWAIT (1 << 0)
+
/**
* extension type
* FUSE_MAX_NR_SECCTX: maximum value of &fuse_secctx_header.nr_secctx
@@ -1075,7 +1081,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 {
