On Thu, Dec 21, 2023 at 12:17 PM Bernd Schubert <bernd.schubert@xxxxxxxxxxx> wrote: > > > > 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. > Ah. right, because open(O_DIRECT) does not enter io cache mode in your branch. I missed that. But still, I think that a better fix for fuse_io_mode would be to treat mmap of O_DIRECT exactly the same as mmap of FOPEN_DIRECT_IO, including invalidate page cache and require FUSE_DIRECT_IO_ALLOW_MMAP. I know this could be a change of behavior of applications doing mmap() on an fd that was opened with O_DIRECT, but I doubt that such applications exist, even if this really works with upstream code. Something like this (pushed to my fuse_io_mode branch)? +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; +} + /* Request access to submit new io to inode via open file */ static bool fuse_file_io_open(struct file *file, struct inode *inode) { @@ -116,7 +121,7 @@ static bool fuse_file_io_open(struct file *file, struct inode *inode) return true; /* Set explicit FOPEN_CACHE_IO flag for file open in caching mode */ - if (!(ff->open_flags & FOPEN_DIRECT_IO) && !(file->f_flags & O_DIRECT)) + if (!fuse_file_is_direct_io(file)) ff->open_flags |= FOPEN_CACHE_IO; spin_lock(&fi->lock); @@ -2622,8 +2627,9 @@ static int fuse_file_mmap(struct file *file, struct vm_area_struct *vma) if (FUSE_IS_DAX(file_inode(file))) return fuse_dax_mmap(file, vma); - if (ff->open_flags & FOPEN_DIRECT_IO) { - /* Can't provide the coherency needed for MAP_SHARED + if (fuse_file_is_direct_io(file)) { + /* + * Can't provide the coherency needed for MAP_SHARED * if FUSE_DIRECT_IO_ALLOW_MMAP isn't set. */ > > > >> > >> > >> 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 ;) > I think that between you and I, fuse_io_mode is getting very close to converging, so queuing it for 6.8 really depends on Miklos' availability during the following week. I suggest that you incorporate my review comments from github and/or use the patches that I pushed to my fuse_io_mode branch and post the io mode patches for review on the list as soon as possible. I could do that, but I trust that you are testing dio much better than I am. > From my point my dio-v5 branch is also ready, it relies on these > patches. Not sure how to post it with the dependency. Basically, you just post the io mode patch set and then you post the dio patches with a reference to the io mode patches that they depend on. > 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). > Feels to me like the dio patches are a bit heavier to review than just the io mode patches, so not likely to be ready for 6.8, but it's not up to me. I can only say that my review of io mode patches is done and that I have tested them, while my own ability to review fuse-dio patches for the 6.8 timeframe is limited. Thanks, Amir.
