Re: [PATCH v2 5/5] fuse: introduce inode io modes

[Miklos Szeredi <miklos@szeredi.hu>]

On Thu, 1 Feb 2024 at 00:09, Bernd Schubert <bschubert@ddn.com> wrote:
>
> From: Amir Goldstein <amir73il@gmail.com>
>
> 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?

>
> 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.


>
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  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.

> +
> +/*
> + * 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.

> +
> +/* 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.