On Thu, Jan 07, 2021 at 04:44:19PM +0200, Amir Goldstein wrote:

On Thu, Jan 7, 2021 at 3:45 PM Vivek Goyal [off-list ref] wrote:

quoted

On Thu, Jan 07, 2021 at 09:02:00AM +0200, Amir Goldstein wrote:

quoted

On Wed, Jan 6, 2021 at 9:47 PM Vivek Goyal [off-list ref] wrote:

quoted

On Wed, Jan 06, 2021 at 12:35:46AM -0800, Sargun Dhillon wrote:

quoted

Overlayfs's volatile option allows the user to bypass all forced sync calls
to the upperdir filesystem. This comes at the cost of safety. We can never
ensure that the user's data is intact, but we can make a best effort to
expose whether or not the data is likely to be in a bad state.

The best way to handle this in the time being is that if an overlayfs's
upperdir experiences an error after a volatile mount occurs, that error
will be returned on fsync, fdatasync, sync, and syncfs. This is
contradictory to the traditional behaviour of VFS which fails the call
once, and only raises an error if a subsequent fsync error has occurred,
and been raised by the filesystem.

One awkward aspect of the patch is that we have to manually set the
superblock's errseq_t after the sync_fs callback as opposed to just
returning an error from syncfs. This is because the call chain looks
something like this:

sys_syncfs ->
      sync_filesystem ->
              __sync_filesystem ->
                      /* The return value is ignored here
                      sb->s_op->sync_fs(sb)
                      _sync_blockdev
              /* Where the VFS fetches the error to raise to userspace */
              errseq_check_and_advance

Because of this we call errseq_set every time the sync_fs callback occurs.

Why not start capturing return code of ->sync_fs and then return error
from ovl->sync_fs. And then you don't have to do errseq_set(ovl_sb).

I already posted a patch to capture retrun code from ->sync_fs.

https://lore.kernel.org/linux-fsdevel/20201221195055.35295-2-vgoyal@redhat.com/ (local)

Vivek,

IMO the more important question is "Why not?".

Your patches will undoubtedly get to mainline in the near future and they do
make the errseq_set(ovl_sb) in this patch a bit redundant,

I thought my patch of capturing ->sync_fs is really simple (just few
lines), so backportability should not be an issue. That's why I
asked for it.

Apologies. I thought you meant your entire patch set.
I do agree to that. In fact, I think I suggested it myself at one
point or another.

quoted

quoted

but I really see no
harm in it. It is very simple for you to remove this line in your patch.
I do see the big benefit of an independent patch that is easy to apply to fix
a fresh v5.10 feature.

I think it is easy for people to dismiss the importance of "syncfs on volatile"
which sounds like a contradiction, but it is not.
The fact that the current behavior is documented doesn't make it right either.
It just makes our review wrong.
The durability guarantee (that volatile does not provide) is very different
from the "reliability" guarantee that it CAN provide.
We do not want to have to explain to people that "volatile" provided different
guarantees depending on the kernel they are running.
Fixing syncfs/fsync of volatile is much more important IMO than erroring
on other fs ops post writeback error, because other fs ops are equally
unreliable on any filesystem in case application did not do fsync.

Ignoring the factor of "backporting cost" when there is no engineering
justification to do so is just ignoring the pain of others.
Do you have an engineering argument for objecting this patch is
applied before your fixes to syncfs vfs API?

Carrying ->sync_fs return code patch is definitely not a blocker. It
is just nice to have. Anyway, I you don't want to carry that ->sync_fs
return patch in stable, I am fine with this patch. I will follow up
on that fix separately.

Please collaborate with Sargun.
I think it is best if one of you will post those two patches in the same
series. I think you had a few minor comments to address, so many
send the final patch version to Sargun to he can test the two patches
together and post them?

Hi Amir,

I was thinking more about that patch. That patch will start returning
error on syncfs() in cases where it did not return errors in the past and 
somebody might complain. So it probably is safer to carry that patch
in mainline first and once it gets good testing, push it to stable later.

So for now, I am fine with this patch as it is. Will follow on ->sync_fs
error capture patch separately. And once that is upstream, I can post
another overlay patch to remove errseq_set().

Sorry for the confusion.
Too many "the syncfs patch" to juggle.

No worries. I agree, too many mail threads on this topic.

Thanks
Vivek