Re: [PATCH v7 3/3] fuse: remove tmp folio for writebacks and internal rb tree

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 4/16/25 18:43, Joanne Koong wrote:
> On Tue, Apr 15, 2025 at 6:40 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>
>> On 4/15/25 11:59 PM, Joanne Koong wrote:
>>> On Tue, Apr 15, 2025 at 12:49 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>>>
>>>> Hi Joanne,
>>>>
>>>> Sorry for the late reply...
>>>
>>> Hi Jingbo,
>>>
>>> No worries at all.
>>>>
>>>>
>>>> On 4/11/25 12:11 AM, Joanne Koong wrote:
>>>>> On Thu, Apr 10, 2025 at 8:11 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>>>>>
>>>>>> On 4/10/25 11:07 PM, Joanne Koong wrote:
>>>>>>> On Wed, Apr 9, 2025 at 7:12 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 4/10/25 7:47 AM, Joanne Koong wrote:
>>>>>>>>>    On Tue, Apr 8, 2025 at 7:43 PM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>>>>>>>>>
>>>>>>>>>> Hi Joanne,
>>>>>>>>>>
>>>>>>>>>> On 4/5/25 2:14 AM, Joanne Koong wrote:
>>>>>>>>>>> In the current FUSE writeback design (see commit 3be5a52b30aa
>>>>>>>>>>> ("fuse: support writable mmap")), a temp page is allocated for every
>>>>>>>>>>> dirty page to be written back, the contents of the dirty page are copied over
>>>>>>>>>>> to the temp page, and the temp page gets handed to the server to write back.
>>>>>>>>>>>
>>>>>>>>>>> This is done so that writeback may be immediately cleared on the dirty page,
>>>>>>>>>>> and this in turn is done in order to mitigate the following deadlock scenario
>>>>>>>>>>> that may arise if reclaim waits on writeback on the dirty page to complete:
>>>>>>>>>>> * single-threaded FUSE server is in the middle of handling a request
>>>>>>>>>>>    that needs a memory allocation
>>>>>>>>>>> * memory allocation triggers direct reclaim
>>>>>>>>>>> * direct reclaim waits on a folio under writeback
>>>>>>>>>>> * the FUSE server can't write back the folio since it's stuck in
>>>>>>>>>>>    direct reclaim
>>>>>>>>>>>
>>>>>>>>>>> With a recent change that added AS_WRITEBACK_INDETERMINATE and mitigates
>>>>>>>>>>> the situations described above, FUSE writeback does not need to use
>>>>>>>>>>> temp pages if it sets AS_WRITEBACK_INDETERMINATE on its inode mappings.
>>>>>>>>>>>
>>>>>>>>>>> This commit sets AS_WRITEBACK_INDETERMINATE on the inode mappings
>>>>>>>>>>> and removes the temporary pages + extra copying and the internal rb
>>>>>>>>>>> tree.
>>>>>>>>>>>
>>>>>>>>>>> fio benchmarks --
>>>>>>>>>>> (using averages observed from 10 runs, throwing away outliers)
>>>>>>>>>>>
>>>>>>>>>>> Setup:
>>>>>>>>>>> sudo mount -t tmpfs -o size=30G tmpfs ~/tmp_mount
>>>>>>>>>>>   ./libfuse/build/example/passthrough_ll -o writeback -o max_threads=4 -o source=~/tmp_mount ~/fuse_mount
>>>>>>>>>>>
>>>>>>>>>>> fio --name=writeback --ioengine=sync --rw=write --bs={1k,4k,1M} --size=2G
>>>>>>>>>>> --numjobs=2 --ramp_time=30 --group_reporting=1 --directory=/root/fuse_mount
>>>>>>>>>>>
>>>>>>>>>>>          bs =  1k          4k            1M
>>>>>>>>>>> Before  351 MiB/s     1818 MiB/s     1851 MiB/s
>>>>>>>>>>> After   341 MiB/s     2246 MiB/s     2685 MiB/s
>>>>>>>>>>> % diff        -3%          23%         45%
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>>>>>>>>>> Reviewed-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>>>>>>>>>>> Acked-by: Miklos Szeredi <mszeredi@redhat.com>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Hi Jingbo,
>>>>>>>>>
>>>>>>>>> Thanks for sharing your analysis for this.
>>>>>>>>>
>>>>>>>>>> Overall this patch LGTM.
>>>>>>>>>>
>>>>>>>>>> Apart from that, IMO the fi->writectr and fi->queued_writes mechanism is
>>>>>>>>>> also unneeded then, at least the DIRECT IO routine (i.e.
>>>>>>>>>
>>>>>>>>> I took a look at fi->writectr and fi->queued_writes and my
>>>>>>>>> understanding is that we do still need this. For example, for
>>>>>>>>> truncates (I'm looking at fuse_do_setattr()), I think we still need to
>>>>>>>>> prevent concurrent writeback or else the setattr request and the
>>>>>>>>> writeback request could race which would result in a mismatch between
>>>>>>>>> the file's reported size and the actual data written to disk.
>>>>>>>>
>>>>>>>> I haven't looked into the truncate routine yet.  I will see it later.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> fuse_direct_io()) doesn't need fuse_sync_writes() anymore.  That is
>>>>>>>>>> because after removing the temp page, the DIRECT IO routine has already
>>>>>>>>>> been waiting for all inflight WRITE requests, see
>>>>>>>>>>
>>>>>>>>>> # DIRECT read
>>>>>>>>>> generic_file_read_iter
>>>>>>>>>>    kiocb_write_and_wait
>>>>>>>>>>      filemap_write_and_wait_range
>>>>>>>>>
>>>>>>>>> Where do you see generic_file_read_iter() getting called for direct io reads?
>>>>>>>>
>>>>>>>> # DIRECT read
>>>>>>>> fuse_file_read_iter
>>>>>>>>    fuse_cache_read_iter
>>>>>>>>      generic_file_read_iter
>>>>>>>>        kiocb_write_and_wait
>>>>>>>>         filemap_write_and_wait_range
>>>>>>>>        a_ops->direct_IO(),i.e. fuse_direct_IO()
>>>>>>>>
>>>>>>>
>>>>>>> Oh I see, I thought files opened with O_DIRECT automatically call the
>>>>>>> .direct_IO handler for reads/writes but you're right, it first goes
>>>>>>> through .read_iter / .write_iter handlers, and the .direct_IO handler
>>>>>>> only gets invoked through generic_file_read_iter() /
>>>>>>> generic_file_direct_write() in mm/filemap.c
>>>>>>>
>>>>>>> There's two paths for direct io in FUSE:
>>>>>>> a) fuse server sets fi->direct_io = true when a file is opened, which
>>>>>>> will set the FOPEN_DIRECT_IO bit in ff->open_flags on the kernel side
>>>>>>> b) fuse server doesn't set fi->direct_io = true, but the client opens
>>>>>>> the file with O_DIRECT
>>>>>>>
>>>>>>> We only go through the stack trace you listed above for the b) case.
>>>>>>> For the a) case, we'll hit
>>>>>>>
>>>>>>>          if (ff->open_flags & FOPEN_DIRECT_IO)
>>>>>>>                  return fuse_direct_read_iter(iocb, to);
>>>>>>>
>>>>>>> and
>>>>>>>
>>>>>>>          if (ff->open_flags & FOPEN_DIRECT_IO)
>>>>>>>                  return fuse_direct_write_iter(iocb, from);
>>>>>>>
>>>>>>> which will invoke fuse_direct_IO() / fuse_direct_io() without going
>>>>>>> through the kiocb_write_and_wait() -> filemap_write_and_wait_range() /
>>>>>>> kiocb_invalidate_pages() -> filemap_write_and_wait_range() you listed
>>>>>>> above.
>>>>>>>
>>>>>>> So for the a) case I think we'd still need the fuse_sync_writes() in
>>>>>>> case there's still pending writeback.
>>>>>>>
>>>>>>> Do you agree with this analysis or am I missing something here?
>>>>>>
>>>>>> Yeah, that's true.  But instead of calling fuse_sync_writes(), we can
>>>>>> call filemap_wait_range() or something similar here.
>>>>>>
>>>>>
>>>>> Agreed. Actually, the more I look at this, the more I think we can
>>>>> replace all fuse_sync_writes() and get rid of it entirely.
>>>>
>>>>
>>>> I have seen your latest reply that this cleaning up won't be included in
>>>> this series, which is okay.
>>>>
>>>>
>>>>> fuse_sync_writes() is called in:
>>>>>
>>>>> fuse_fsync():
>>>>>          /*
>>>>>           * Start writeback against all dirty pages of the inode, then
>>>>>           * wait for all outstanding writes, before sending the FSYNC
>>>>>           * request.
>>>>>           */
>>>>>          err = file_write_and_wait_range(file, start, end);
>>>>>          if (err)
>>>>>                  goto out;
>>>>>
>>>>>          fuse_sync_writes(inode);
>>>>>
>>>>>          /*
>>>>>           * Due to implementation of fuse writeback
>>>>>           * file_write_and_wait_range() does not catch errors.
>>>>>           * We have to do this directly after fuse_sync_writes()
>>>>>           */
>>>>>          err = file_check_and_advance_wb_err(file);
>>>>>          if (err)
>>>>>                  goto out;
>>>>>
>>>>>
>>>>>        We can get rid of the fuse_sync_writes() and
>>>>> file_check_and_advance_wb_err() entirely since now without temp pages,
>>>>> the file_write_and_wait_range() call actually ensures that writeback
>>>>> is completed
>>>>>
>>>>>
>>>>>
>>>>> fuse_writeback_range():
>>>>>          static int fuse_writeback_range(struct inode *inode, loff_t
>>>>> start, loff_t end)
>>>>>          {
>>>>>                  int err =
>>>>> filemap_write_and_wait_range(inode->i_mapping, start, LLONG_MAX);
>>>>>
>>>>>                  if (!err)
>>>>>                          fuse_sync_writes(inode);
>>>>>
>>>>>                  return err;
>>>>>          }
>>>>>
>>>>>
>>>>>        We can replace fuse_writeback_range() entirely with
>>>>> filemap_write_and_wait_range().
>>>>>
>>>>>
>>>>>
>>>>> fuse_direct_io():
>>>>>          if (fopen_direct_io && fc->direct_io_allow_mmap) {
>>>>>                  res = filemap_write_and_wait_range(mapping, pos, pos +
>>>>> count - 1);
>>>>>                  if (res) {
>>>>>                          fuse_io_free(ia);
>>>>>                          return res;
>>>>>                  }
>>>>>          }
>>>>>          if (!cuse && filemap_range_has_writeback(mapping, pos, (pos +
>>>>> count - 1))) {
>>>>>                  if (!write)
>>>>>                          inode_lock(inode);
>>>>>                  fuse_sync_writes(inode);
>>>>>                  if (!write)
>>>>>                          inode_unlock(inode);
>>>>>          }
>>>>>
>>>>>
>>>>>         I think this can just replaced with
>>>>>                  if (fopen_direct_io && (fc->direct_io_allow_mmap || !cuse)) {
>>>>>                          res = filemap_write_and_wait_range(mapping,
>>>>> pos, pos + count - 1);
>>>>>                          if (res) {
>>>>>                                  fuse_io_free(ia);
>>>>>                                  return res;
>>>>>                          }
>>>>>                  }
>>>>
>>>> Alright. But I would prefer doing this filemap_write_and_wait_range() in
>>>> fuse_direct_write_iter() rather than fuse_direct_io() if possible.
>>>>
>>>>>         since for the !fopen_direct_io case, it will already go through
>>>>> filemap_write_and_wait_range(), as you mentioned in your previous
>>>>> message. I think this also fixes a bug (?) in the original code - in
>>>>> the fopen_direct_io && !fc->direct_io_allow_mmap case, I think we
>>>>> still need to write out dirty pages first, which we don't currently
>>>>> do.
>>>>
>>>> Nope.  In case of fopen_direct_io && !fc->direct_io_allow_mmap, there
>>>> won't be any page cache at all, right?
>>>>
>>>
>>> Isn't there still a page cache if the file was previously opened
>>> without direct io and then the client opens another handle to that
>>> file with direct io? In that case, the pages could still be dirty in
>>> the page cache and would need to be written back first, no?
>>
>> Do you mean that when the inode is firstly opened, FOPEN_DIRECT_IO is
>> not set by the FUSE server, while it is secondly opened, the flag is set?
>>
>> Though the behavior of the FUSE daemon is quite confusing in this case,
>> it is completely possible in real life.  So yes we'd better add
>> filemap_write_and_wait_range() unconditionally in fopen_direct_io case.
>>
> 
> I think this behavior on the server side is pretty common. From what
> I've seen on most servers, the server when handling the open sets
> fi->direct_io depending on if the client opens with O_DIRECT, eg
> 
>          if (fi->flags & O_DIRECT)
>                  fi->direct_io = 1;

One should do that, actually, to get a shared lock on the inode.
With the additional

fi.parallel_direct_writes = 1;

> 
> If a client opens a file without O_DIRECT and then opens the same file
> with O_DIRECT, then we run into this case. Though I'm not sure how
> common it generally is for clients to do this.
> 


I guess the common case is

application1 does mmap
application2 does normal read/write

fuse-server might set  fi->direct_io = 1 on all opens, with the
additional

fuse_set_feature_flag(connp, FUSE_CAP_DIRECT_IO_ALLOW_MMAP);

Probably will only come to it tomorrow, but will review,
especially to check about cached/uncached io-modes.



Thanks,
Bernd