On Sat, Dec 19, 2020 at 07:53:12AM -0500, Jeff Layton wrote:
> On Sat, 2020-12-19 at 06:13 +0000, Matthew Wilcox wrote:
> > On Thu, Dec 17, 2020 at 10:00:37AM -0500, Jeff Layton wrote:
> > > Overlayfs's volatile mounts want to be able to sample an error for their
> > > own purposes, without preventing a later opener from potentially seeing
> > > the error.
> >
> > umm ... can't they just copy the errseq_t they're interested in, followed
> > by calling errseq_check() later?
> >
>
> They don't want the sampling for the volatile mount to prevent later
> openers from seeing an error that hasn't yet been reported.
>
> If they copy the errseq_t (or just do an errseq_sample), and then follow
> it with a errseq_check_and_advance then the SEEN bit will end up being
> set and a later opener wouldn't see the error.
>
> Aside from that though, I think this patch clarifies things a bit since
> the SEEN flag currently means two different things:
>
> 1/ do I need to increment the counter when recording another error?
>
> 2/ do I need to report this error to new samplers (at open time)
>
> That was ok before, since we those conditions were always changed
> together, but with the overlayfs volatile mount use-case, it no longer
> does.
>
> > actually, isn't errseq_check() buggy in the face of multiple
> > watchers? consider this:
> >
> > worker.es starts at 0
> > t2.es = errseq_sample(&worker.es)
> > errseq_set(&worker.es, -EIO)
> > t1.es = errseq_sample(&worker.es)
> > t2.err = errseq_check_and_advance(&es, t2.es)
> > ** this sets ERRSEQ_SEEN **
> > t1.err = errseq_check(&worker.es, t1.es)
> > ** reports an error, even though the only change is that
> > ERRSEQ_SEEN moved **.
> >
> > i think errseq_check() should be:
> >
> > if (likely(cur | ERRSEQ_SEEN) == (since | ERRSEQ_SEEN))
> > return 0;
> >
> > i'm not yet convinced other changes are needed to errseq. but i am
> > having great trouble understanding exactly what overlayfs is trying to do.
>
> I think you're right on errseq_check. I'll plan to do a patch to fix
> that up as well.
>
> I too am having a bit of trouble understanding all of the nuances here.
> My current understanding is that it depends on the "volatility" of the
> mount:
>
> normal (non-volatile): they basically want to be able to track errors as
> if the files were being opened on the upper layer. For this case I think
> they should aim to just do all of the error checking against the upper
> sb and ignore the overlayfs s_wb_err field. This does mean pushing the
> errseq_check_and_advance down into the individual filesystems in some
> fashion though.
>
> volatile: they want to sample at mount time and always return an error
> to syncfs if there has been another error since the original sample
> point. This sampling should also not affect later openers on the upper
> layer (or on other overlayfs mounts).
I think syncfs() for both volatile and non-volatile mounts should
probably be treated same way and existing erreseq API should be
good enough for that.
So say overlayfs installs file->f_sb_err at file open time.
ovl_file->f_sb_err = errseq_sample(&upper_sb->s_wb_err)
And at syncfs() time either file system specific ->sync_fs routine
or a separate ->sb_errseq_check() routine does following
ret = errseq_check_and_advance(&upper_sb->s_wb_err, &ovl_file->f_sb_err);
And syncfs() will return error to user space.
So both volatile and non-volatile mount see similar to me. There is
one key difference.
- non-volatile mount does not actually sync the file to disk. So it
it only return error if some other writeback error has been recorded
on upper_sb since file was opened.
So what are the use cases for new errseq API? I think there are
two.
- Sargun has been trying to remount volatile mounts without every calling
syncfs on upper. And he wants to detect if there has been an error
since volatile mount was last mounted/unmounted. In this case he
wants to take a snapshot of current state of upper_sb->s_wb_err
and check this again at remount time to make sure there are not
any new errors since he took snapshot.
He does not want to consume the UNSEEN error in this process because
it can potentially lead to complaints from normal syncfs users
saying that sheer act of mounting an overlayfs consumed UNSEEN
error on upper sb. They would expect it to be consumed when one
of the callers does syncfs() either on upper->sb directly or
indirectly through overlay mount.
So IMHO, that's the core reason behind the new errseq API.
- Once we have it, I think we can potentialy use it at other places
in overlayfs to check for errors. For example, at overlay read()
routines might want to check if there has been an error recorded
on upper since volatile overlay was mounted. If yes, fail read
with -EIO and fail the filesystem so all the future reads
return -EIO.
That's one thing different with volatile overlayfs. Typically
if applications write something to a file, they can do.
write(fd)
fsync(fd)
read(fd)
And this should make sure apps are protected against writeback
errors.
But with volatile mounts, we actually don't do any fsync(). So it
is possible that fysnc() returns success and when read() happens
we either read data we wrote or we read old data from disk (because
writeback failed and pagecache page was reclaimed).
To avoid this situation with volatile overlay mounts, we thought
that we will fail overlay volatile mount as soon as first writeback
error is detected.
Now syncfs() is just only one of the sample points. We should be able
to check for writeback errors in other paths like mount/read/write
etc and fail volatile fs. And in all these paths, we don't want
to mark error as SEEN otherwise user space might complain.
In summary, with volatile overlay use case, there is a need to
check for errors outside the syncfs() (mount/read/write/...). If
we check for errors in these paths we have two alternatives.
- Either mark unseen error on upper_sb as SEEN. And then user space
can complain that an unseen error was consumed without anybody
ever calling syncfs.
- Or we introduce new errseq API which allows checking for errors
without marking it SEEN. That way we can detect errors at the
same time unseen error will be reported to user space through
syncfs().
And second option seems to be safer choice, that's why errseq_peek()
patches.
> I'm not 100% clear on whether I understand both use-cases correctly
> though.
I hope, above is a decent summary of various discussions we have had
so far.
Vivek
