On Mon, 4 Dec 2023 at 07:50, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> 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.
I've done that. Thanks for the fix and testing.
Miklos
