Re: [PATCH v2 0/3] Check errors on sync for volatile overlayfs mounts

[Jeff Layton <jlayton@kernel.org>]

On Fri, 2020-12-11 at 15:49 -0800, Sargun Dhillon wrote:
> The semantics of errseq and syncfs are such that it is impossible to track
> if any errors have occurred between the time the first error occurred, and
> the user checks for the error (calls syncfs, and subsequently
> errseq_check_and_advance.
> 
> Overlayfs has a volatile feature which short-circuits syncfs. This, in turn
> makes it so that the user can have silent data corruption and not know
> about it. The third patch in the series introduces behaviour that makes it
> so that we can track errors, and bubble up whether the user has put
> themselves in bad situation.
> 
> This required some gymanstics in errseq, and adding a wrapper around it
> called "errseq_counter" (errseq + counter). The data structure uses an
> atomic to track overflow errors. This approach, rather than moving to an
> atomic64 / u64 is so we can avoid bloating every person that subscribes to
> an errseq, and only add the subscriber behaviour to those who care (at the
> expense of space.
> 
> The datastructure is write-optimized, and rightfully so, as the users
> of the counter feature are just overlayfs, and it's called in fsync
> checking, which is a rather seldom operation, and not really on
> any hotpaths.
> 
> [1]: https://lore.kernel.org/linux-fsdevel/20201202092720.41522-1-sargun@sargun.me/
> 
> Sargun Dhillon (3):
>   errseq: Add errseq_counter to allow for all errors to be observed
>   errseq: Add mechanism to snapshot errseq_counter and check snapshot
>   overlay: Implement volatile-specific fsync error behaviour
> 
>  Documentation/filesystems/overlayfs.rst |   8 ++
>  fs/buffer.c                             |   2 +-
>  fs/overlayfs/file.c                     |   5 +-
>  fs/overlayfs/overlayfs.h                |   1 +
>  fs/overlayfs/ovl_entry.h                |   3 +
>  fs/overlayfs/readdir.c                  |   5 +-
>  fs/overlayfs/super.c                    |  26 +++--
>  fs/overlayfs/util.c                     |  28 +++++
>  fs/super.c                              |   1 +
>  fs/sync.c                               |   3 +-
>  include/linux/errseq.h                  |  18 ++++
>  include/linux/fs.h                      |   6 +-
>  include/linux/pagemap.h                 |   2 +-
>  lib/errseq.c                            | 129 ++++++++++++++++++++----
>  14 files changed, 202 insertions(+), 35 deletions(-)
> 

It would hel if you could more clearly lay out the semantics you're
looking for. If I understand correctly:

You basically want to be able to sample the sb->s_wb_err of the upper
layer at mount time and then always return an error if any new errors
were recorded since that point.

If that's correct, then I'm not sure I get need for all of this extra
counter machinery. Why not just sample it at mount time without
recording it as 0 if the seen flag isn't set. Then just do an
errseq_check against the upper superblock (without advancing) in the
overlayfs ->sync_fs routine and just errseq_set that error into the
overlayfs superblock? The syncfs syscall wrapper should then always
report the latest error.

Or (even better) rework all of the sync_fs/syncfs mess to be more sane,
so that overlayfs has more control over what errors get returned to
userland. ISTM that the main problem you have is that the
errseq_check_and_advance is done in the syscall wrapper, and that's
probably not appropriate for your use-case.

-- 
Jeff Layton <jlayton@kernel.org>