On 2/1/24 17:33, Amir Goldstein wrote:
> 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.
I had also missed that fcntl option :/
>
> 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?
Yeah, currently FOPEN_PARALLEL_DIRECT_WRITES does not have an effect
without FOPEN_DIRECT_IO. In my fuse-dio consolidation branch that
changes. For me it is ok that parallel dio requires
FOPEN_DIRECT_IO, easy enough to set on the server side in open methods
when O_DIRECT is set. We should just document it in fuse.h.
>
> 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.
Note to myself, I need to make double sure that our (Dharmendras/mine)
atomic open branch gets updated.
Thanks,
Bernd
