Re: [PATCH 1/3] fuse: fix wrong ff->iomode state changes from parallel dio write

[Miklos Szeredi <miklos@szeredi.hu>]

On Tue, 9 Apr 2024 at 17:10, Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Apr 9, 2024 at 4:33 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sun, 7 Apr 2024 at 17:58, Amir Goldstein <amir73il@gmail.com> wrote:
> > >
> > > There is a confusion with fuse_file_uncached_io_{start,end} interface.
> > > These helpers do two things when called from passthrough open()/release():
> > > 1. Take/drop negative refcount of fi->iocachectr (inode uncached io mode)
> > > 2. State change ff->iomode IOM_NONE <-> IOM_UNCACHED (file uncached open)
> > >
> > > The calls from parallel dio write path need to take a reference on
> > > fi->iocachectr, but they should not be changing ff->iomode state,
> > > because in this case, the fi->iocachectr reference does not stick around
> > > until file release().
> >
> > Okay.
> >
> > >
> > > Factor out helpers fuse_inode_uncached_io_{start,end}, to be used from
> > > parallel dio write path and rename fuse_file_*cached_io_{start,end}
> > > helpers to fuse_file_*cached_io_{open,release} to clarify the difference.
> > >
> > > Add a check of ff->iomode in mmap(), so that fuse_file_cached_io_open()
> > > is called only on first mmap of direct_io file.
> >
> > Is this supposed to be an optimization?
>
> No.
> The reason I did this is because I wanted to differentiate
> the refcount semantics (start/end)
> from the state semantics (open/release)
> and to make it clearer that there is only one state change
> and refcount increment on the first mmap().
>
> > AFAICS it's wrong, because it
> > moves the check outside of any relevant locks.
> >
>
> Aren't concurrent mmap serialized on some lock?

Only on current->mm, which doesn't serialize mmaps of the same file in
different processes.

>
> Anyway, I think that the only "bug" that this can trigger is the
> WARN_ON(ff->iomode != IOM_NONE)
> so if we ....
>
> >
> > > @@ -56,8 +57,7 @@ int fuse_file_cached_io_start(struct inode *inode, struct fuse_file *ff)
> > >                 return -ETXTBSY;
> > >         }
> > >
> > > -       WARN_ON(ff->iomode == IOM_UNCACHED);
> > > -       if (ff->iomode == IOM_NONE) {
> > > +       if (!WARN_ON(ff->iomode != IOM_NONE)) {
> >
> > This double negation is ugly.  Just let the compiler optimize away the
> > second comparison.
>
> ...drop this change, we should be good.
>
> If you agree, do you need me to re-post?

Okay, but then what's the point of the unlocked check?

Thanks,
Miklos