On 12/21/23 10:18, Amir Goldstein wrote:
On Thu, Dec 21, 2023 at 12:13 AM Bernd Schubert
<bernd.schubert@xxxxxxxxxxx> wrote:
[...]
I think that we are going to need to use some inode state flag
(e.g. FUSE_I_DIO_WR_EXCL) to protect against this starvation,
unless we do not care about this possibility?
We'd only need to set this in fuse_file_io_mmap() until we get
the iocachectr refcount.
I added back FUSE_I_CACHE_IO_MODE I had used previously.
ACK.
Name is a bit confusing for the "want io mode" case, but IMO
a comment would be enough to make it clear.
Push a version with a comment to my branch.
I *think* that fuse_inode_deny_io_cache() should be called with
shared inode lock held, because of the existing lock chain
i_rwsem -> page lock -> mmap_lock for page faults, but I am
not sure. My brain is too cooked now to figure this out.
OTOH, I don't see any problem with calling
fuse_inode_deny_io_cache() with shared lock held?
I pushed this version to my fuse_io_mode branch [1].
Only tested generic/095 with FOPEN_DIRECT_IO and
DIRECT_IO_ALLOW_MMAP.
Thanks,
Amir.
[1] https://github.com/amir73il/linux/commits/fuse_io_mode
Thanks, will look into your changes next. I was looking into the
initial
issue with generic/095 with my branch. Fixed by the attached patch. I
think it is generic and also applies to FOPEN_DIRECT_IO + mmap.
Interesting is that filemap_range_has_writeback() is exported, but
there
was no user. Hopefully nobody submits an unexport patch in the mean
time.
Ok. Now I am pretty sure that filemap_range_has_writeback() should be
check after taking the shared lock in fuse_dio_lock() as in my branch
and
not in fuse_dio_wr_exclusive_lock() outside the lock.
But at the same time, it is a little concerning that you are able to
observe
dirty pages on a fuse inode after success of fuse_inode_deny_io_cache().
The whole point of fuse_inode_deny_io_cache() is that it should be
granted after all users of the inode page cache are gone.
Is it expected that fuse inode pages remain dirty after no more open
files
and no more mmaps?
I'm actually not sure anymore if filemap_range_has_writeback() is
actually needed. In fuse_flush() it calls write_inode_now(inode, 1),
but I don't think that will flush queued fi->writectr
(fi->writepages). Will report back in the afternoon.
Sorry, my fault, please ignore the previous patch. Actually no dirty
pages to be expected, I had missed the that fuse_flush calls
fuse_sync_writes(). The main bug in my branch was due to the different
handling of FOPEN_DIRECT_IO and O_DIRECT - for O_DIRECT I hadn't called
fuse_file_io_mmap().
But why would you need to call fuse_file_io_mmap() for O_DIRECT?
If a file was opened without FOPEN_DIRECT_IO, we already set inode to
caching mode on open.
Does your O_DIRECT patch to mmap solve an actual reproducible bug?
Yeah it does, in my fuse-dio-v5 branch, which adds in shared locks for
O_DIRECT writes without FOPEN_DIRECT_IO.
I pushed a few fixes/updates into my fuse-dio-v5 branch and also to
simplify it for you to my fuse_io_mode branch. Changes are onto of the
previous patches io-mode patch to simplify it for you to see the changes
and to possibly squash it into the main io patch.
https://github.com/bsbernd/linux/commits/fuse_io_mode/
Cool. I squashed all your fixes to my branch, with minor comments
that I also left on github, except for the O_DIRECT patch, because
I do not understand why it is needed.
No issue with that, I can keep that patch on the branch that actually
needs it.
Oh, I just see your comments - I didn't get github notification and so
missed your comments before. Sorry about that. Checking where I need to
enable it. I do get notifications for other projects, so didn't suspect
that anything would be missing...
The 6.8 merge window is very close and the holidays are upon us,
so not sure if you and Miklos could be bothered, but do you think there
is a chance that we can get fuse_io_mode patches ready for queuing
in time for the 6.8 merge window?
They do have merit on their own for re-allowing parallel dio along with
FOPEN_PARALLEL_DIRECT_WRITES, but also, it would make it easier
for the both of us to develop fuse-dio and fuse-passthrough based on
the io cache mode during the 6.9 dev cycle.
I definitely would also like to get these patches in. Holidays have the
merit that I don't need to get up at 7am to wake up kids and am then
tired all the day. And no meetings ;)
From my point my dio-v5 branch is also ready, it relies on these
patches. Not sure how to post it with the dependency.
I also have no issue to wait for 6.9, for now I'm going to take these
patches to our fuse module for ubuntu and rhel9 kernels (quite heavily
patched, as it needs to live aside the kernel included module - symbol
renames, etc).
PS: I start to feel a bit guilty about this long thread on
linux-fsdevel. Would be better to have that on fuse-devel, just the
sourceforge list is badly spammed.
According to MAINTAINERS, linux-fsdevel is the list for linux FUSE
kernel development. The sourceforge fuse-devel is for libfuse.
We could open a linux-fuse list, but it has been this way forever
and I do not know of any complaints from fsdevel members.
the downside of not having linux-fuse list IMO is that we do not
have a "fuse only" searchable archive, but we won't have it for all the
historic messages on fsdevel anyway.
Sure, fine with me. I'm just a bit worried that others might get
disturbed by all the fuse only messages.
Thanks,
Bernd
