On 8/25/20 01:31, Amir Goldstein wrote: > On Tue, Aug 25, 2020 at 3:55 AM Vivek Goyal <vgoyal@xxxxxxxxxx> wrote: >> On Tue, Aug 25, 2020 at 12:51:55AM +0300, Amir Goldstein wrote: >>>> Ok, I am wondering why are we concerned about older kernels. I mean, >>>> if we introduce new features, we don't provide compatibility with >>>> older kernels. Say "metacopy", "redirect_dir". If you mount with >>>> older kernel, they will see something which you don't expect. >>>> >>> True. We missed the opportunity to do the work/incompat trick >>> with metacopy etc. >>> >>>> So why "volatile" is different. We seem to be bending backward and >>>> using an unrelated behavior of overlay to provide this. >>>> >>>> Why not simply drop a file $workdir/volatile for volatile mounts >>>> and check for presence of this file when mounting? >>>> >>> That's an option. >>> But what's the problem with >>> $workdir/work/incompat/volatile/dirty >>> compared to: >>> $workdir/volatile >>> >>> It's not more complicated to implement is it? >>> So we get some extra protection with little extra cost. No? >> Ok, I will look into it. >>> I don't feel strongly about it. >>> >>> But I must say, according to Giuseppe's description of the use case: >>> "mount volatile overlay+umount overlay+syncfs upper dir..." >>> looks like what he is looking for is "volatile,sync=shutdown", is it not? >>> >>> And if this is the case, I think it would be much preferred to implement >>> "volatile,sync=shutdown", over documenting how to make a "volatile" >>> overlay mountable from outside overlay. Don't you guys agree? >> When it comes to requirements, to me it felt that Giuseppe seemed >> to have two requirements. For running containers, he did not care >> seem to care about syncing upper to disk at all. For building >> images he probably wanted to sync upper to disk. >> > You know, I am not sure that building images requires syncing to disk. > Why is syncfs() needed between unmount and copying/tar'ing the layers? > Why is it needed before mounting again? It is not. > It is only needed "before crash", so whether or not "dirty volatile" overlay > can be mounted is a decision better be made by userspace. > > The only problem with this approach is that it is a bit harder to document > the filesystem behavior, but I think that we need to. > >> From overlayfs perspective, "volatile,sync=shutdown" seems like >> a nicer interface because overlay will take care of removing >> "dirty" file and until and unless crash happens, user does >> not have to step in and there is less confusion about syncing >> upper and removing dirty file etc. >> >> Last time Miklos seemed to prefer to implement just "volatile" >> for now and drop "sync=shutdown". >> >> https://lore.kernel.org/linux-unionfs/CAJfpegt2k=r6TRok57tKPcLyUhCBOcBAV7bgLSPrQYXsPoPkpQ@xxxxxxxxxxxxxx/ >> >> I personally think that "volatile,sync=shutdown" is first good step >> because it is less error prone and overlayfs manages dirty file >> and it will provide lot of benefits in terms of not having to >> do very frequent sync. >> >> And if this does not prove to be enough for certain use cases, >> then one can extend this to also implement "volatile,sync=none". >> >> But frankly speaking, there has been so much of back and forth >> on this patch, that I am fine with any of the option which is >> acceptable to Miklos. >> > I agree. > Miklos accepted $workdir/work/incompat/volatile/dirty. > I assume the name 'dirty'/'donotremove' is not an issue. > It's simple. > Let's go with that. > > Thanks, > Amir. > I don't believe we need the sync=shutdown. But I am fine with adding it. in both cases we are looking at using this. Buildah for building container images and CRI-O where Kubernetes comes in and blows away all of the containers on system restart, we don't care about data in the upper directory on a crash.
