On Mon, Dec 4, 2023 at 1:00 AM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
>
> Hi Amir,
>
> On 12/3/23 12:20, Amir Goldstein wrote:
> > On Sat, Dec 2, 2023 at 5:06 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >>
> >> On Mon, Nov 6, 2023 at 4:08 PM Bernd Schubert
> >> <bernd.schubert@xxxxxxxxxxx> wrote:
> >>>
> >>> Hi Miklos,
> >>>
> >>> On 9/20/23 10:15, Miklos Szeredi wrote:
> >>>> On Wed, 20 Sept 2023 at 04:41, Tyler Fanelli <tfanelli@xxxxxxxxxx> wrote:
> >>>>>
> >>>>> At the moment, FUSE_INIT's DIRECT_IO_RELAX flag only serves the purpose
> >>>>> of allowing shared mmap of files opened/created with DIRECT_IO enabled.
> >>>>> However, it leaves open the possibility of further relaxing the
> >>>>> DIRECT_IO restrictions (and in-effect, the cache coherency guarantees of
> >>>>> DIRECT_IO) in the future.
> >>>>>
> >>>>> The DIRECT_IO_ALLOW_MMAP flag leaves no ambiguity of its purpose. It
> >>>>> only serves to allow shared mmap of DIRECT_IO files, while still
> >>>>> bypassing the cache on regular reads and writes. The shared mmap is the
> >>>>> only loosening of the cache policy that can take place with the flag.
> >>>>> This removes some ambiguity and introduces a more stable flag to be used
> >>>>> in FUSE_INIT. Furthermore, we can document that to allow shared mmap'ing
> >>>>> of DIRECT_IO files, a user must enable DIRECT_IO_ALLOW_MMAP.
> >>>>>
> >>>>> Tyler Fanelli (2):
> >>>>> fs/fuse: Rename DIRECT_IO_RELAX to DIRECT_IO_ALLOW_MMAP
> >>>>> docs/fuse-io: Document the usage of DIRECT_IO_ALLOW_MMAP
> >>>>
> >>>> Looks good.
> >>>>
> >>>> Applied, thanks. Will send the PR during this merge window, since the
> >>>> rename could break stuff if already released.
> >>>
> >>> I'm just porting back this feature to our internal fuse module and it
> >>> looks these rename patches have been forgotten?
> >>>
> >>>
> >>
> >> Hi Miklos, Bernd,
> >>
> >> I was looking at the DIRECT_IO_ALLOW_MMAP code and specifically at
> >> commit b5a2a3a0b776 ("fuse: write back dirty pages before direct write in
> >> direct_io_relax mode") and I was wondering - isn't dirty pages writeback
> >> needed *before* invalidate_inode_pages2() in fuse_file_mmap() for
> >> direct_io_allow_mmap case?
> >>
> >> For FUSE_PASSTHROUGH, I am going to need to call fuse_vma_close()
> >> for munmap of files also in direct-io mode [1], so I was considering installing
> >> fuse_file_vm_ops for the FOPEN_DIRECT_IO case, same as caching case,
> >> and regardless of direct_io_allow_mmap.
> >>
> >> I was asking myself if there was a good reason why fuse_page_mkwrite()/
> >> fuse_wait_on_page_writeback()/fuse_vma_close()/write_inode_now()
> >> should NOT be called for the FOPEN_DIRECT_IO case regardless of
> >> direct_io_allow_mmap?
> >>
> >
> > Before trying to make changes to fuse_file_mmap() I tried to test
> > DIRECT_IO_RELAX - I enabled it in libfuse and ran fstest with
> > passthrough_hp --direct-io.
> >
> > The test generic/095 - "Concurrent mixed I/O (buffer I/O, aiodio, mmap, splice)
> > on the same files" blew up hitting BUG_ON(fi->writectr < 0) in
> > fuse_set_nowrite()
> >
> > I am wondering how this code was tested?
> >
> > I could not figure out the problem and how to fix it.
> > Please suggest a fix and let me know which adjustments are needed
> > if I want to use fuse_file_vm_ops for all mmap modes.
>
> So fuse_set_nowrite() tests for inode_is_locked(), but that also
> succeeds for a shared lock. It gets late here (and I might miss
> something), but I think we have an issue with
> FOPEN_PARALLEL_DIRECT_WRITES. Assuming there would be plain O_DIRECT and
> mmap, the same issue might triggered? Hmm, well, so far plain O_DIRECT
> does not support FOPEN_PARALLEL_DIRECT_WRITES yet - the patches for that
> are still pending.
>
Your analysis seems to be correct.
Attached patch fixes the problem and should be backported to 6.6.y.
Miklos,
I prepared the patch on top of master and not on top of the rename to
FUSE_DIRECT_IO_ALLOW_MMAP in for-next for ease of backport to
6.6.y, although if you are planning send the flag rename to v6.7 as a fix,
you may prefer to apply the fix after the rename and request to backport
the flag rename along with the fix to 6.6.y.
Having the final flag name in v6.6.y would be a nice bonus.
Let me know if you want me to post the fix patch based on for-next.
Thanks,
Amir.
From 46865e2660d0e9d64cd8c56c905eafee7f4c03b5 Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Sun, 3 Dec 2023 09:42:33 +0200
Subject: [PATCH] fuse: disable FOPEN_PARALLEL_DIRECT_WRITES with
FUSE_DIRECT_IO_RELAX
The new fuse init flag FUSE_DIRECT_IO_RELAX breaks assumptions made by
FOPEN_PARALLEL_DIRECT_WRITES and causes test generic/095 to hit
BUG_ON(fi->writectr < 0) assertions in fuse_set_nowrite():
generic/095 5s ...
kernel BUG at fs/fuse/dir.c:1756!
...
? fuse_set_nowrite+0x3d/0xdd
? do_raw_spin_unlock+0x88/0x8f
? _raw_spin_unlock+0x2d/0x43
? fuse_range_is_writeback+0x71/0x84
fuse_sync_writes+0xf/0x19
fuse_direct_io+0x167/0x5bd
fuse_direct_write_iter+0xf0/0x146
Auto disable FOPEN_PARALLEL_DIRECT_WRITES when server negotiated
FUSE_DIRECT_IO_RELAX.
Fixes: e78662e818f9 ("fuse: add a new fuse init flag to relax restrictions in no cache mode")
Cc: <stable@xxxxxxxxxxxxxxx> # v6.6
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
fs/fuse/file.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 1cdb6327511e..5b5297805675 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1574,6 +1574,7 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
ssize_t res;
bool exclusive_lock =
!(ff->open_flags & FOPEN_PARALLEL_DIRECT_WRITES) ||
+ get_fuse_conn(inode)->direct_io_relax ||
iocb->ki_flags & IOCB_APPEND ||
fuse_direct_write_extending_i_size(iocb, from);
@@ -1581,6 +1582,7 @@ static ssize_t fuse_direct_write_iter(struct kiocb *iocb, struct iov_iter *from)
* Take exclusive lock if
* - Parallel direct writes are disabled - a user space decision
* - Parallel direct writes are enabled and i_size is being extended.
+ * - Shared mmap on direct_io file is supported (FUSE_DIRECT_IO_RELAX).
* This might not be needed at all, but needs further investigation.
*/
if (exclusive_lock)
--
2.34.1
