On Tue, Jan 5, 2021 at 6:26 PM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: > > On Tue, Jan 05, 2021 at 09:11:23AM +0200, Amir Goldstein wrote: > > > > > > > > What I would rather see is: > > > > - Non-volatile: first syncfs in every container gets an error (nice to have) > > > > > > I am not sure why are we making this behavior per container. This should > > > be no different from current semantics we have for syncfs() on regular > > > filesystem. And that will provide what you are looking for. If you > > > want single error to be reported in all ovleray mounts, then make > > > sure you have one fd open in each mount after mount, then call syncfs() > > > on that fd. > > > > > > > Ok. > > > > > Not sure why overlayfs behavior/semantics should be any differnt > > > than what regular filessytems like ext4/xfs are offering. Once we > > > get page cache sharing sorted out with xfs reflink, then people > > > will not even need overlayfs and be able to launch containers > > > just using xfs reflink and share base image. In that case also > > > they will need to keep an fd open per container they want to > > > see an error in. > > > > > > So my patches exactly provide that. syncfs() behavior is same with > > > overlayfs as application gets it on other filesystems. And to me > > > its important to keep behavior same. > > > > > > > - Volatile: every syncfs and every fsync in every container gets an error > > > > (important IMO) > > > > > > For volatile mounts, I agree that we need to fail overlayfs instance > > > as soon as first error is detected since mount. And this applies to > > > not only syncfs()/fsync() but to read/write and other operations too. > > > > > > For that we will need additional patches which are floating around > > > to keep errseq sample in overlay and check for errors in all > > > paths syncfs/fsync/read/write/.... and fail fs. > > > > > But these patches build on top of my patches. > > > > Here we disagree. > > > > I don't see how Jeff's patch is "building on top of your patches" > > seeing that it is perfectly well contained and does not in fact depend > > on your patches. > > Jeff's patches are solving problem only for volatile mounts and they > are propagating error to overlayfs sb. > > My patches are solving the issue both for volatile mount as well as > non-volatile mounts and solve it using same method so there is no > confusion. > > So there are multiple pieces to this puzzle and IMHO, it probably > should be fixed in this order. > > A. First fix the syncfs() path to return error both for volatile as > as well non-volatile mounts. > > B. And then add patches to fail filesystem for volatile mount as soon > as first error is detected (either in syncfs path or in other paths > like read/write/...). This probably will require to save errseq > in ovl_fs, and then compare with upper_sb in critical paths and fail > filesystem as soon as error is detected. > > C. Finally fix the issues related to mount/remount error detection which > Sargun is wanting to fix. This will be largerly solved by B except > saving errseq on disk. > > My patches should fix the first problem. And more patches can be > applied on top to fix issue B and issue C. > > Now if we agree with this, in this context I see that fixing problem > B and C is building on top of my patches which fixes problem A. > That order is fine by me. > > > > And I do insist that the fix for volatile mounts syncfs/fsync error > > reporting should be applied before your patches or at the very least > > not heavily depend on them. > > I still don't understand that why volatile syncfs() error reporting > is more important than non-volatile syncfs(). But I will stop harping > on this point now. > > My issue with Jeff's patches is that syncfs() error reporting should > be dealt in same way both for volatile and non-volatile mount. That > is compare file->f_sb_err and upper_sb->s_wb_err to figure out if > there is an error to report to user space. Currently this patches > only solve the problem for volatile mounts and use propagation to > overlay sb which is conflicting for non-volatile mounts. > > IIUC, your primary concern with volatile mount is that you want to > detect as soon as writeback error happens, and flag it to container > manager so that container manager can stop container, throw away > upper layer and restart from scratch. If yes, what you want can > be solved by solving problem B and backporting it to LTS kernel. > I think patches for that will be well contained within overlayfs > (And no VFS) changes and should be relatively easy to backport. > > IOW, backportability to LTS kernel should not be a concern/blocker > for my patch series which fixes syncfs() issue for overlayfs. > That's all I wanted to know. Thanks, Amir.
