Re: [PATCH v14 00/12] FUSE passthrough for file io

From: Amir Goldstein <amir73il@gmail.com>
To: Bernd Schubert <bernd.schubert@fastmail.fm>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	Daniel Rosenberg <drosen@google.com>,
	 Paul Lawrence <paullawrence@google.com>,
	Alessio Balsini <balsini@android.com>,
	 Christian Brauner <brauner@kernel.org>,
	fuse-devel@lists.sourceforge.net,  linux-fsdevel@vger.kernel.org,
	Dave Chinner <david@fromorbit.com>
Subject: Re: [PATCH v14 00/12] FUSE passthrough for file io
Date: Thu, 7 Dec 2023 09:23:38 +0200	[thread overview]
Message-ID: <CAOQ4uxh1UWY_OLV1Zq-phFXcOFVp=EJHtgZdXB021Fr-ZHPWzg@mail.gmail.com> (raw)
In-Reply-To: <f224ffac-c59e-47dd-8e11-721d7b1c7104@fastmail.fm>

On Thu, Dec 7, 2023 at 1:11 AM Bernd Schubert
<bernd.schubert@fastmail.fm> wrote:
>
> Hi Amir,
>
>
> On 12/6/23 10:59, Amir Goldstein wrote:
> > On Wed, Nov 29, 2023 at 6:55 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >>
> >> On Wed, 29 Nov 2023 at 16:52, Amir Goldstein <amir73il@gmail.com> wrote:
> >>
> >>> direct I/O read()/write() is never a problem.
> >>>
> >>> The question is whether mmap() on a file opened with FOPEN_DIRECT_IO
> >>> when the inode is in passthrough mode, also uses fuse_passthrough_mmap()?
> >>
> >> I think it should.
> >>
> >>> or denied, similar to how mmap with ff->open_flags & FOPEN_DIRECT_IO &&
> >>> vma->vm_flags & VM_MAYSHARE) && !fc->direct_io_relax
> >>> is denied?
> >>
> >> What would be the use case for FOPEN_DIRECT_IO with passthrough mmap?
> >>
> >>> A bit more challenging, because we will need to track unmounts, or at
> >>> least track
> >>> "was_cached_mmaped" state per file, but doable.
> >>
> >> Tracking unmaps via fuse_vma_close() should not be difficult.
> >>
> >
> > I think that it is.
> >
> > fuse_vma_close() does not seem to be balanced with fuse_file_mmap()
> > because IIUC, maps can be cloned via fork() etc.
> >
> > It tried to implement an iocachectr refcount to track cache mmaps,
> > but it keeps underflowing in fuse_vma_close().
> >
> > I would like us to consider a slightly different model.
> >
> > We agreed that caching and passthrough mode on the same
> > inode cannot mix and there is no problem with different modes
> > per inode on the same filesystem.
> >
> > I have a use case for mixing direct_io and passthrough on the
> > same inode (i.e. inode in passthrough mode).
> >
> > I have no use case (yet) for the transition from caching to passthrough
> > mode on the same inode and direct_io cached mmaps complicate
> > things quite a bit for this scenario.
> >
> > My proposal is to taint a direct_io file with FOPEN_CACHE_MMAP
> > if it was ever mmaped using page cache.
> > We will not try to clean this flag in fuse_vma_close(), it stays with
> > the file until release.
> >
> > An FOPEN_CACHE_MMAP file forces an inode into caching mode,
> > same as a regular caching open.
>
> where do you actually want to set that flag? My initial idea for
> FUSE_I_CACHE_WRITES was to set that in fuse_file_mmap, but I would have
> needed the i_rwsem lock and that resulted in a lock ordering issue.
>

Yes, the idea is to set this flag on the first mmap of a FOPEN_DIRECT_IO file.
Why does that require an i_rwsem lock?

Before setting FUSE_I_CACHE_WRITES inode flag, your patch does:
    wait_event(fi->direct_io_waitq, fi->shared_lock_direct_io_ctr == 0);

We can do the same in direct_io mmap, before setting the
FOPEN_CACHE_MMAP file flag and consequently changing inode
mode to caching. No?

I will try to prepare a patch today to demonstrate.

Thanks,
Amir.