Re: [PATCH v5] overlayfs: Provide a mount option "volatile" to skip sync

[Daniel Walsh <dwalsh@redhat.com>]

On 8/25/20 01:31, Amir Goldstein wrote:
> On Tue, Aug 25, 2020 at 3:55 AM Vivek Goyal <vgoyal@redhat.com> 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@mail.gmail.com/
>>
>> 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.