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?
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?
Thanks,
Amir.
