On Fri, Feb 9, 2024 at 12:21 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Thu, 8 Feb 2024 at 18:09, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> > We use an internal FOPEN_CACHE_IO flag to mark a file that was open in
> > caching mode. This flag can not be set by the server.
> >
> > 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.
>
> I think using FOPEN_CACHE_IO for this is wrong. Why not just an internal flag?
>
No reason apart from "symmetry" with the other io mode FOPEN flags.
I will use an internal flag.
> > +/* No more pending io and no new io possible to inode via open/mmapped file */
> > +void fuse_file_io_release(struct fuse_file *ff, struct inode *inode)
> > +{
> > + if (!ff->io_opened)
> > + return;
> > +
> > + /* Last caching file close exits caching inode io mode */
> > + if (ff->open_flags & FOPEN_CACHE_IO)
> > + fuse_file_cached_io_end(inode);
>
> The above is the only place where io_opened is checked without a
> WARN_ON. So can't this be incorporated into the flag indicating
> whether caching is on?
I added io_open to fix a bug in an earlier version where fuse_file_release()
was called on the error path after fuse_file_io_open() failed.
The simple change would be to replace FOPEN_CACHE_IO flag
with ff->io_cache_opened bit.
I assume you meant to change ff->io_opened to an enum:
{ FUSE_IO_NONE, FUSE_IO_CACHE, FUSE_IO_DIRECT,
FUSE_IO_PASSTHROUGH }
Is that what you mean?
Thanks,
Amir.
