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? > > > 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. 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. > > 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. Thanks, Amir.
