On Thu, Feb 1, 2024 at 4:47 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Thu, 1 Feb 2024 at 00:09, Bernd Schubert <bschubert@xxxxxxx> wrote:
> >
> > From: Amir Goldstein <amir73il@xxxxxxxxx>
> >
> > The fuse inode io mode is determined by the mode of its open files/mmaps
> > and parallel dio.
> >
> > - caching io mode - files open in caching mode or mmap on direct_io file
> > - direct io mode - no files open in caching mode and no files mmaped
> > - parallel dio mode - direct io mode with parallel dio in progress
>
> Specifically if iocachectr is:
>
> > 0 -> caching io
> == 0 -> direct io
> < 0 -> parallel io
>
> >
> > We use a new FOPEN_CACHE_IO flag to explicitly mark a file that was open
> > in caching mode.
>
> This is really confusing. FOPEN_CACHE_IO is apparently an internally
> used flag, but it's defined on the userspace API.
>
> a) what is the meaning of this flag on the external API?
> b) what is the purpose of this flag internally?
The purpose is to annotate the state of direct io file that was mmaped
as FOPEN_DIRECT_IO | FOPEN_CACHE_IO.
An fd like this puts inode in caching mode and its release may get inode
out of caching mode.
I did not manage to do refcoutning with fuse_vma_close(), because those
calls are not balances with fuse_file_mmap() calls.
The first mmap() of an FOPEN_DIRECT_IO file may incur wait for completion
of parallel dio.
The only use of exporting FOPEN_CACHE_IO to the server is that it could
force incurring this wait at open() time instead of mmap() time.
I also considered for servers that advertise FUSE_PASSTHROUGH
capability to not allow open without specifying an explicit io mode,
that is one of FOPEN_DIRECT_IO | FOPEN_CACHE_IO |
FOPEN_PASSTHROUGH, but I did not actually implement those
semantics ATM.
>
> >
> > direct_io mmap uses page cache, so first mmap will mark the file as
> > FOPEN_DIRECT_IO|FOPEN_CACHE_IO (i.e. mixed mode) and inode will enter
> > the caching io mode.
> >
> > If the server opens the file with flags FOPEN_DIRECT_IO|FOPEN_CACHE_IO,
> > the inode enters caching io mode already on open.
> >
> > This allows executing parallel dio when inode is not in caching mode
> > even if shared mmap is allowed, but no mmaps have been performed on
> > the inode in question.
> >
> > An open in caching mode and mmap on direct_io file now waits for all
> > in-progress parallel dio writes to complete, so paralle dio writes
> > together with FUSE_DIRECT_IO_ALLOW_MMAP is enabled by this commit.
>
> I think the per file state is wrong, the above can be done with just
> the per-inode state.
>
The per-file state is to taint the file is "has been mmaped".
I did not find another way of doing this.
Please suggest another way.
>
> >
> > Signed-off-by: Bernd Schubert <bschubert@xxxxxxx>
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> > fs/fuse/file.c | 215 ++++++++++++++++++++++++++++++++++++--
> > fs/fuse/fuse_i.h | 79 +++++++++++++-
> > include/uapi/linux/fuse.h | 2 +
> > 3 files changed, 286 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> > index 7d2f4b0eb36a..eb9929ff9f60 100644
> > --- a/fs/fuse/file.c
> > +++ b/fs/fuse/file.c
> > @@ -105,10 +105,177 @@ static void fuse_release_end(struct fuse_mount *fm, struct fuse_args *args,
> > kfree(ra);
> > }
> >
> > +static bool fuse_file_is_direct_io(struct file *file)
> > +{
> > + struct fuse_file *ff = file->private_data;
> > +
> > + return ff->open_flags & FOPEN_DIRECT_IO || file->f_flags & O_DIRECT;
> > +}
>
> This is one of the issues with the per-file state. O_DIRECT can be
> changed with fcntl(fd, F_SETFL, ...), so state calculated at open can
> go stale.
>
Ouch!
Actually, it may not matter much if we just ignore O_DIRECT
completely for the purpose of io modes.
The io mode could be determined solely from FMODE_* flags.
Worst case is that if the server opens an O_DIRECT file without
FOPEN_DIRECT_IO, then this inode cannot do parallel dio
and this inode cannot be opened in passthrough mode.
I don't think that is such a big problem.
Bernd, what do you think?
BTW, server can and probably should open O_DIRECT files
with FOPEN_PASSTHROUGH and that means something
completely different than O_DIRECT && FOPEN_DIRECT_IO -
it means that io is passed through to the backing file without
doing buffered io on the backing file.
> > +
> > +/*
> > + * Wait for cached io to be allowed -
> > + * Blocks new parallel dio writes and waits for the in-progress parallel dio
> > + * writes to complete.
> > + */
> > +static int fuse_inode_wait_for_cached_io(struct fuse_inode *fi)
> > +{
> > + int err = 0;
> > +
> > + assert_spin_locked(&fi->lock);
> > +
> > + while (!err && !fuse_inode_get_io_cache(fi)) {
> > + /*
> > + * Setting the bit advises new direct-io writes
> > + * to use an exclusive lock - without it the wait below
> > + * might be forever.
> > + */
> > + set_bit(FUSE_I_CACHE_IO_MODE, &fi->state);
> > + spin_unlock(&fi->lock);
> > + err = wait_event_killable(fi->direct_io_waitq,
> > + fuse_is_io_cache_allowed(fi));
> > + spin_lock(&fi->lock);
> > + }
> > + /* Clear FUSE_I_CACHE_IO_MODE flag if failed to enter caching mode */
> > + if (err && fi->iocachectr <= 0)
> > + clear_bit(FUSE_I_CACHE_IO_MODE, &fi->state);
> > +
> > + return err;
> > +}
>
> I suggest moving all infrastructure, including the inline helpers in
> fuse_i.h into a separate source file.
>
OK. I can make those changes.
> > +
> > +/* Start cached io mode where parallel dio writes are not allowed */
> > +static int fuse_file_cached_io_start(struct inode *inode)
> > +{
> > + struct fuse_inode *fi = get_fuse_inode(inode);
> > + int err;
> > +
> > + spin_lock(&fi->lock);
> > + err = fuse_inode_wait_for_cached_io(fi);
> > + spin_unlock(&fi->lock);
> > + return err;
> > +}
> > +
> > +static void fuse_file_cached_io_end(struct inode *inode)
> > +{
> > + struct fuse_inode *fi = get_fuse_inode(inode);
> > +
> > + spin_lock(&fi->lock);
> > + fuse_inode_put_io_cache(get_fuse_inode(inode));
> > + spin_unlock(&fi->lock);
> > +}
> > +
> > +/* Start strictly uncached io mode where cache access is not allowed */
> > +static int fuse_file_uncached_io_start(struct inode *inode)
> > +{
> > + struct fuse_inode *fi = get_fuse_inode(inode);
> > + bool ok;
> > +
> > + spin_lock(&fi->lock);
> > + ok = fuse_inode_deny_io_cache(fi);
> > + spin_unlock(&fi->lock);
> > + return ok ? 0 : -ETXTBSY;
> > +}
> > +
> > +static void fuse_file_uncached_io_end(struct inode *inode)
> > +{
> > + struct fuse_inode *fi = get_fuse_inode(inode);
> > + bool allow_cached_io;
> > +
> > + spin_lock(&fi->lock);
> > + allow_cached_io = fuse_inode_allow_io_cache(fi);
> > + spin_unlock(&fi->lock);
> > + if (allow_cached_io)
> > + wake_up(&fi->direct_io_waitq);
> > +}
> > +
> > +/* Open flags to determine regular file io mode */
> > +#define FOPEN_IO_MODE_MASK \
> > + (FOPEN_DIRECT_IO | FOPEN_CACHE_IO)
> > +
> > +/* Request access to submit new io to inode via open file */
> > +static int fuse_file_io_open(struct file *file, struct inode *inode)
> > +{
> > + struct fuse_file *ff = file->private_data;
> > + int iomode_flags = ff->open_flags & FOPEN_IO_MODE_MASK;
> > + int err;
> > +
> > + err = -EBUSY;
> > + if (WARN_ON(ff->io_opened))
> > + goto fail;
> > +
> > + if (!S_ISREG(inode->i_mode) || FUSE_IS_DAX(inode)) {
>
> This S_ISREG check can also go away with separating the directory opens.
OK. I will see if I can make that cleanup as well.
Thanks,
Amir.
