On Tue, Apr 9, 2024 at 7:18 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote:
>
> 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.
Miklos,
I see that you removed the unlocked check in the fix staged for-next.
Please also remove this from commit message:
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.
Thanks,
Amir.
