On Tue, Apr 9, 2024 at 6:32 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Tue, 9 Apr 2024 at 17:10, Amir Goldstein <amir73il@xxxxxxxxx> wrote:
> >
> > On Tue, Apr 9, 2024 at 4:33 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
> > >
> > > On Sun, 7 Apr 2024 at 17:58, Amir Goldstein <amir73il@xxxxxxxxx> 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?
As I wrote, I just did it to emphasize the open-once
semantics.
If you do not like the unlocked check, feel free to remove it.
Thanks,
Amir.
