[PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[Amir Goldstein]

Miklos,

I implemented freeze/thaw of overlayfs, because I need it for
overlay snapshots (CoW decision is made before mnt_wat_write(upper)
and I need to serialize it with snapshot take).

Not sure if there are other use cases for overlayfs freeze??

Tested freeze stress with xfstest generic/068 generic/390.

While staring at the code, I realized that syncfs(2) for overlayfs
seems broken.  It looks like only inodes are synced and upper fs
metadata is not being flushed, but I could be wrong.

Tested sync sanity with -g quick (although no test calls syncfs directly).
Tested the usual unionmount sanity over xfs and over tmpfs.

I am not sure exactly how to write a test case to verify this alleged
breakage?

Jan,

Can you please have a look?

Thanks,

Amir.

Amir Goldstein (2):
  ovl: support freeze/thaw super
  ovl: properly implement sync_filesystem()

 fs/overlayfs/super.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

-- 
2.7.4

[PATCH 1/2] ovl: support freeze/thaw super

[Amir Goldstein]

freeze/thaw of upper is all that is needed.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 6792bb7..0f8a9c8 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -160,6 +160,20 @@ static void ovl_put_super(struct super_block *sb)
 	kfree(ufs);
 }
 
+static int ovl_freeze(struct super_block *sb)
+{
+	struct ovl_fs *ufs = sb->s_fs_info;
+
+	return ufs->upper_mnt ? freeze_super(ufs->upper_mnt->mnt_sb) : 0;
+}
+
+static int ovl_unfreeze(struct super_block *sb)
+{
+	struct ovl_fs *ufs = sb->s_fs_info;
+
+	return ufs->upper_mnt ? thaw_super(ufs->upper_mnt->mnt_sb) : 0;
+}
+
 /**
  * ovl_statfs
  * @sb: The overlayfs super block
@@ -222,6 +236,8 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
 
 static const struct super_operations ovl_super_operations = {
 	.put_super	= ovl_put_super,
+	.freeze_fs	= ovl_freeze,
+	.unfreeze_fs	= ovl_unfreeze,
 	.statfs		= ovl_statfs,
 	.show_options	= ovl_show_options,
 	.remount_fs	= ovl_remount,
-- 
2.7.4

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Amir Goldstein]

On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> freeze/thaw of upper is all that is needed.
>

Miklos,

Looking at it again, I believe that not even that is needed.
Having fixed ovl_sync_fs() with patch #2, ovl_freeze()
and ovl_unfreeze() need to be NOP. Am I right?

In fact, freezing upper fs, when many overlayfs mounts
share the same base fs (a-la docker) would be quite lame.

WRT other use cases of freezing overlayfs, I am not sure
if it is needed for correctness of docker checkpoint/restart?

Pavel?

> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/overlayfs/super.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 6792bb7..0f8a9c8 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -160,6 +160,20 @@ static void ovl_put_super(struct super_block *sb)
>         kfree(ufs);
>  }
>
> +static int ovl_freeze(struct super_block *sb)
> +{
> +       struct ovl_fs *ufs = sb->s_fs_info;
> +
> +       return ufs->upper_mnt ? freeze_super(ufs->upper_mnt->mnt_sb) : 0;
> +}
> +
> +static int ovl_unfreeze(struct super_block *sb)
> +{
> +       struct ovl_fs *ufs = sb->s_fs_info;
> +
> +       return ufs->upper_mnt ? thaw_super(ufs->upper_mnt->mnt_sb) : 0;
> +}
> +
>  /**
>   * ovl_statfs
>   * @sb: The overlayfs super block
> @@ -222,6 +236,8 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
>
>  static const struct super_operations ovl_super_operations = {
>         .put_super      = ovl_put_super,
> +       .freeze_fs      = ovl_freeze,
> +       .unfreeze_fs    = ovl_unfreeze,
>         .statfs         = ovl_statfs,
>         .show_options   = ovl_show_options,
>         .remount_fs     = ovl_remount,
> --
> 2.7.4
>

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Pavel Emelyanov]

On 01/23/2017 11:47 AM, Amir Goldstein wrote:
> On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> freeze/thaw of upper is all that is needed.
>>
> 
> Miklos,
> 
> Looking at it again, I believe that not even that is needed.
> Having fixed ovl_sync_fs() with patch #2, ovl_freeze()
> and ovl_unfreeze() need to be NOP. Am I right?
> 
> In fact, freezing upper fs, when many overlayfs mounts
> share the same base fs (a-la docker) would be quite lame.
> 
> WRT other use cases of freezing overlayfs, I am not sure
> if it is needed for correctness of docker checkpoint/restart?
> 
> Pavel?

Thanks for the heads-up :)

Good question. Actually we haven't yet experimented with doing _full_
(with FS) snapshot of containers, but at the first glance I can't
see the need for OVL freeze :( We stop all the processes in entry.S
effectively, so no in-flight IO can be happening.

>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/super.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 6792bb7..0f8a9c8 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -160,6 +160,20 @@ static void ovl_put_super(struct super_block *sb)
>>         kfree(ufs);
>>  }
>>
>> +static int ovl_freeze(struct super_block *sb)
>> +{
>> +       struct ovl_fs *ufs = sb->s_fs_info;
>> +
>> +       return ufs->upper_mnt ? freeze_super(ufs->upper_mnt->mnt_sb) : 0;
>> +}
>> +
>> +static int ovl_unfreeze(struct super_block *sb)
>> +{
>> +       struct ovl_fs *ufs = sb->s_fs_info;
>> +
>> +       return ufs->upper_mnt ? thaw_super(ufs->upper_mnt->mnt_sb) : 0;
>> +}
>> +
>>  /**
>>   * ovl_statfs
>>   * @sb: The overlayfs super block
>> @@ -222,6 +236,8 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
>>
>>  static const struct super_operations ovl_super_operations = {
>>         .put_super      = ovl_put_super,
>> +       .freeze_fs      = ovl_freeze,
>> +       .unfreeze_fs    = ovl_unfreeze,
>>         .statfs         = ovl_statfs,
>>         .show_options   = ovl_show_options,
>>         .remount_fs     = ovl_remount,
>> --
>> 2.7.4
>>
> .
> 

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Pavel Emelyanov]

On 01/23/2017 11:47 AM, Amir Goldstein wrote:
> On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> freeze/thaw of upper is all that is needed.
>>
> 
> Miklos,
> 
> Looking at it again, I believe that not even that is needed.
> Having fixed ovl_sync_fs() with patch #2, ovl_freeze()
> and ovl_unfreeze() need to be NOP. Am I right?
> 
> In fact, freezing upper fs, when many overlayfs mounts
> share the same base fs (a-la docker) would be quite lame.
> 
> WRT other use cases of freezing overlayfs, I am not sure
> if it is needed for correctness of docker checkpoint/restart?
> 
> Pavel?

Thanks for the heads-up :)

Good question. Actually we haven't yet experimented with doing _full_
(with FS) snapshot of containers, but at the first glance I can't
see the need for OVL freeze :( We stop all the processes in entry.S
effectively, so no in-flight IO can be happening.

>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>> ---
>>  fs/overlayfs/super.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
>> index 6792bb7..0f8a9c8 100644
>> --- a/fs/overlayfs/super.c
>> +++ b/fs/overlayfs/super.c
>> @@ -160,6 +160,20 @@ static void ovl_put_super(struct super_block *sb)
>>         kfree(ufs);
>>  }
>>
>> +static int ovl_freeze(struct super_block *sb)
>> +{
>> +       struct ovl_fs *ufs = sb->s_fs_info;
>> +
>> +       return ufs->upper_mnt ? freeze_super(ufs->upper_mnt->mnt_sb) : 0;
>> +}
>> +
>> +static int ovl_unfreeze(struct super_block *sb)
>> +{
>> +       struct ovl_fs *ufs = sb->s_fs_info;
>> +
>> +       return ufs->upper_mnt ? thaw_super(ufs->upper_mnt->mnt_sb) : 0;
>> +}
>> +
>>  /**
>>   * ovl_statfs
>>   * @sb: The overlayfs super block
>> @@ -222,6 +236,8 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
>>
>>  static const struct super_operations ovl_super_operations = {
>>         .put_super      = ovl_put_super,
>> +       .freeze_fs      = ovl_freeze,
>> +       .unfreeze_fs    = ovl_unfreeze,
>>         .statfs         = ovl_statfs,
>>         .show_options   = ovl_show_options,
>>         .remount_fs     = ovl_remount,
>> --
>> 2.7.4
>>
> .
> 

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Amir Goldstein]

On Mon, Jan 23, 2017 at 3:23 PM, Pavel Emelyanov <xemul@virtuozzo.com> wrote:
> On 01/23/2017 11:47 AM, Amir Goldstein wrote:
>> On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>>> freeze/thaw of upper is all that is needed.
>>>
>>
>> Miklos,
>>
>> Looking at it again, I believe that not even that is needed.
>> Having fixed ovl_sync_fs() with patch #2, ovl_freeze()
>> and ovl_unfreeze() need to be NOP. Am I right?
>>
>> In fact, freezing upper fs, when many overlayfs mounts
>> share the same base fs (a-la docker) would be quite lame.
>>
>> WRT other use cases of freezing overlayfs, I am not sure
>> if it is needed for correctness of docker checkpoint/restart?
>>
>> Pavel?
>
> Thanks for the heads-up :)
>
> Good question. Actually we haven't yet experimented with doing _full_
> (with FS) snapshot of containers, but at the first glance I can't
> see the need for OVL freeze :( We stop all the processes in entry.S
> effectively, so no in-flight IO can be happening.
>

Serge, Stephan,

Same question.

Would lxc-snapshot gain anything from the ability to fsfreeze an overlay
mount?

Basically, it should give you the ability to create a consistent snapshot
of overlayfs upper dir from a running container, but maybe you already do
that by freezing the container processes?
I couldn't figure that out from lxc-snapshot documentation.

The context is that I implemented overlayfs fsfreeze for my own needs,
but I need other use cases to justify merging the feature.

Thanks,
Amir.

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Serge E. Hallyn]

Quoting Amir Goldstein (amir73il@gmail.com):
> On Mon, Jan 23, 2017 at 3:23 PM, Pavel Emelyanov <xemul@virtuozzo.com> wrote:
> > On 01/23/2017 11:47 AM, Amir Goldstein wrote:
> >> On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> >>> freeze/thaw of upper is all that is needed.
> >>>
> >>
> >> Miklos,
> >>
> >> Looking at it again, I believe that not even that is needed.
> >> Having fixed ovl_sync_fs() with patch #2, ovl_freeze()
> >> and ovl_unfreeze() need to be NOP. Am I right?
> >>
> >> In fact, freezing upper fs, when many overlayfs mounts
> >> share the same base fs (a-la docker) would be quite lame.
> >>
> >> WRT other use cases of freezing overlayfs, I am not sure
> >> if it is needed for correctness of docker checkpoint/restart?
> >>
> >> Pavel?
> >
> > Thanks for the heads-up :)
> >
> > Good question. Actually we haven't yet experimented with doing _full_
> > (with FS) snapshot of containers, but at the first glance I can't
> > see the need for OVL freeze :( We stop all the processes in entry.S
> > effectively, so no in-flight IO can be happening.
> >
> 
> Serge, Stephan,
> 
> Same question.
> 
> Would lxc-snapshot gain anything from the ability to fsfreeze an overlay
> mount?

lxc-snapshot only works on stopped containers.  'lxc snapshot' can do live
snapshots using criu.  Tycho, does that do anything right now to freeze the
fs?  I'm not sure that freezing all the tasks is necessarily enough to settle
the fs, but I assume you're doing something about that already?

> Basically, it should give you the ability to create a consistent snapshot
> of overlayfs upper dir from a running container, but maybe you already do
> that by freezing the container processes?
> I couldn't figure that out from lxc-snapshot documentation.
> 
> The context is that I implemented overlayfs fsfreeze for my own needs,
> but I need other use cases to justify merging the feature.
> 
> Thanks,
> Amir.

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Tycho Andersen]

On Tue, Apr 04, 2017 at 12:47:52PM -0500, Serge E. Hallyn wrote:
> > Would lxc-snapshot gain anything from the ability to fsfreeze an overlay
> > mount?
> 
> lxc-snapshot only works on stopped containers.  'lxc snapshot' can do live
> snapshots using criu.  Tycho, does that do anything right now to freeze the
> fs?

Not that I'm aware of (CRIU might, but we don't in liblxc).

> I'm not sure that freezing all the tasks is necessarily enough to settle
> the fs, but I assume you're doing something about that already?

I suspect it's not, but we're not doing anything besides freezing the
tasks. In fact, we freeze the tasks by using the freezer cgroup,
which itself is buggy, since the freezer cgroup can race with various
filesystems. So, freezing tasks is hard, and I haven't even thought
about how to freeze the fs for real :)

But in any case, an fs freezing primitive does sound useful for
checkpoint restore, assuming that we're right and freezing the tasks
is simply not enough.

Tycho

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Amir Goldstein]

On Tue, Apr 4, 2017 at 9:01 PM, Tycho Andersen <tycho@docker.com> wrote:
> On Tue, Apr 04, 2017 at 12:47:52PM -0500, Serge E. Hallyn wrote:
>> > Would lxc-snapshot gain anything from the ability to fsfreeze an overlay
>> > mount?
>>
>> lxc-snapshot only works on stopped containers.  'lxc snapshot' can do live
>> snapshots using criu.  Tycho, does that do anything right now to freeze the
>> fs?
>
> Not that I'm aware of (CRIU might, but we don't in liblxc).
>
>> I'm not sure that freezing all the tasks is necessarily enough to settle
>> the fs, but I assume you're doing something about that already?
>
> I suspect it's not, but we're not doing anything besides freezing the
> tasks. In fact, we freeze the tasks by using the freezer cgroup,
> which itself is buggy, since the freezer cgroup can race with various
> filesystems. So, freezing tasks is hard, and I haven't even thought
> about how to freeze the fs for real :)
>
> But in any case, an fs freezing primitive does sound useful for
> checkpoint restore, assuming that we're right and freezing the tasks
> is simply not enough.
>

So I already asked Pavel that question and he said that freezing
the tasks is enough. I am not convinced it is really enough to bring
a file system image (i.e. underlying blockdev) to a quiescent state,
but I think it may be enough for getting a stable view of the mounted
file system, so the files could be dumped somewhere.
I am guessing is what lxc snapshot does?

I still didn't understand wrt lxc snapshot, is there a use case for
taking live snapshots without using CRIU? (because freezer cgroup
mentioned races or whatnot?).

It's definitely possible with btrfs and if my overlayfs freeze patches
are not terribly wrong, then it should be easy with overlayfs as well.
Does lxc snapshot already support live snapshot of btrfs container?

If there is interest, I can try to do a POC of live lxc snapshot of an
overlayfs container.

Amir.

Re: [PATCH 1/2] ovl: support freeze/thaw super

[Tycho Andersen]

On Tue, Apr 04, 2017 at 09:59:16PM +0300, Amir Goldstein wrote:
> On Tue, Apr 4, 2017 at 9:01 PM, Tycho Andersen <tycho@docker.com> wrote:
> > On Tue, Apr 04, 2017 at 12:47:52PM -0500, Serge E. Hallyn wrote:
> >> > Would lxc-snapshot gain anything from the ability to fsfreeze an overlay
> >> > mount?
> >>
> >> lxc-snapshot only works on stopped containers.  'lxc snapshot' can do live
> >> snapshots using criu.  Tycho, does that do anything right now to freeze the
> >> fs?
> >
> > Not that I'm aware of (CRIU might, but we don't in liblxc).
> >
> >> I'm not sure that freezing all the tasks is necessarily enough to settle
> >> the fs, but I assume you're doing something about that already?
> >
> > I suspect it's not, but we're not doing anything besides freezing the
> > tasks. In fact, we freeze the tasks by using the freezer cgroup,
> > which itself is buggy, since the freezer cgroup can race with various
> > filesystems. So, freezing tasks is hard, and I haven't even thought
> > about how to freeze the fs for real :)
> >
> > But in any case, an fs freezing primitive does sound useful for
> > checkpoint restore, assuming that we're right and freezing the tasks
> > is simply not enough.
> >
> 
> So I already asked Pavel that question and he said that freezing
> the tasks is enough. I am not convinced it is really enough to bring
> a file system image (i.e. underlying blockdev) to a quiescent state,
> but I think it may be enough for getting a stable view of the mounted
> file system, so the files could be dumped somewhere.
> I am guessing is what lxc snapshot does?

Yes, lxc snapshot is basically just a frontend for CRIU.

> I still didn't understand wrt lxc snapshot, is there a use case for
> taking live snapshots without using CRIU? (because freezer cgroup
> mentioned races or whatnot?).

No, I think CRIU is the only project that will ever attempt to do
checkpoint restore this way ;-). CRIU supports two different ways of
freezing tasks: one using the freezer cgroup and one without. The one
without doesn't work against fork bombs very well, and the one with
doesn't work because of some filesystems. So it's mostly a container
engine implementation choice which to use.

> It's definitely possible with btrfs and if my overlayfs freeze patches
> are not terribly wrong, then it should be easy with overlayfs as well.
> Does lxc snapshot already support live snapshot of btrfs container?

Yes, it does. It freezes the tasks via the cgroup freezer and then
does a btrfs snapshot of the filesystem once the tasks are frozen.

Tycho

[PATCH 2/2] ovl: properly implement sync_filesystem()

[Amir Goldstein]

overlayfs syncs all inode pages on sync_filesystem(), but it also
needs to call s_op->sync_fs() of upper fs for metadata sync.

This should fix correctness of syncfs(2) and fsfreeze(8).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/overlayfs/super.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 0f8a9c8..5dc1d42 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -160,6 +160,25 @@ static void ovl_put_super(struct super_block *sb)
 	kfree(ufs);
 }
 
+static int ovl_sync_fs(struct super_block *sb, int wait)
+{
+	struct ovl_fs *ufs = sb->s_fs_info;
+	struct super_block *upper_sb;
+	int ret;
+
+	if (!ufs->upper_mnt)
+		return 0;
+	upper_sb = ufs->upper_mnt->mnt_sb;
+	if (!upper_sb->s_op->sync_fs)
+		return 0;
+
+	/* real inodes have already been synced by sync_filesystem(ovl_sb) */
+	down_read(&upper_sb->s_umount);
+	ret = upper_sb->s_op->sync_fs(upper_sb, wait);
+	up_read(&upper_sb->s_umount);
+	return ret;
+}
+
 static int ovl_freeze(struct super_block *sb)
 {
 	struct ovl_fs *ufs = sb->s_fs_info;
@@ -236,6 +255,7 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
 
 static const struct super_operations ovl_super_operations = {
 	.put_super	= ovl_put_super,
+	.sync_fs	= ovl_sync_fs,
 	.freeze_fs	= ovl_freeze,
 	.unfreeze_fs	= ovl_unfreeze,
 	.statfs		= ovl_statfs,
-- 
2.7.4

Re: [PATCH 2/2] ovl: properly implement sync_filesystem()

[Christoph Hellwig]

s/sync_filesystem()/->sync_fs/ in the subject?

Otherwise this looks fine to me:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/2] ovl: properly implement sync_filesystem()

[Amir Goldstein]

On Tue, Jan 24, 2017 at 7:14 PM, Christoph Hellwig <hch@infradead.org> wrote:
> s/sync_filesystem()/->sync_fs/ in the subject?
>

Sure. Thanks.

Just waiting for feedback from Miklos before I decide if I re-post
with or without
 overlay freeze support.

> Otherwise this looks fine to me:
>
> Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[Amir Goldstein]

On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> I implemented freeze/thaw of overlayfs, because I need it for
> overlay snapshots (CoW decision is made before mnt_wat_write(upper)
> and I need to serialize it with snapshot take).
>

Miklos, Jan,

I would be grateful if you guys could help me clarify something wrt
freezefs vs. remount.
I hope you may be able to enlighten me with just the information
provided in this email
without having to dive into overlayfs snapshot details.

I implemented 'snapshot take' for overlayfs snapshots using a command like:
mount -o remount,snapshot=/snap/N /mnt

The 'snapshot take' moment from overlayfs writers perspective is when
a single pointer
is modified via rcu_assign_pointer(ufs->__snapmnt, snapmnt):

https://github.com/amir73il/linux/commit/f0d6759efb7a6281ecc55dcdf33837568f71d29a

I chose to use the remount_fs() API because it is convenient and make sense from
user POV, but I could also use some IOCTL API for snapshot take.

For consistency of snapshot from application POV, I need to do:
fsfreeze -f
snapshot take
fsfreeze -u

But then I cannot use the remount_fs() API which requires that fs be unfrozen.

Can you please explain why do_remount_sb() requires that fs be unfrozen?

Would it be possible to relax this requirement depending on remount flags?

The reason I ask is because overlay remount does not do any writes nor
call mnt_want_write() for that matter.

Thanks,
Amir.

Re: [PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[Jan Kara]

On Thu 19-01-17 21:03:12, Amir Goldstein wrote:
> On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> I would be grateful if you guys could help me clarify something wrt
> freezefs vs. remount.
> I hope you may be able to enlighten me with just the information
> provided in this email
> without having to dive into overlayfs snapshot details.
> 
> I implemented 'snapshot take' for overlayfs snapshots using a command like:
> mount -o remount,snapshot=/snap/N /mnt
> 
> The 'snapshot take' moment from overlayfs writers perspective is when
> a single pointer
> is modified via rcu_assign_pointer(ufs->__snapmnt, snapmnt):
> 
> https://github.com/amir73il/linux/commit/f0d6759efb7a6281ecc55dcdf33837568f71d29a
> 
> I chose to use the remount_fs() API because it is convenient and make
> sense from user POV, but I could also use some IOCTL API for snapshot
> take.

So I don't see why remounting makes sense for making snapshots. Remounting
is for manipulating mount options, not for instructing filesystem to make a
snapshot. I think using ioctl(2) for manipulation of snapshots like btrfs
does makes more sense (and it is also consistent with the existing
precedens of btrfs).

> For consistency of snapshot from application POV, I need to do:
> fsfreeze -f
> snapshot take
> fsfreeze -u
> 
> But then I cannot use the remount_fs() API which requires that fs be unfrozen.
> 
> Can you please explain why do_remount_sb() requires that fs be unfrozen?

I think initially it was because we expected filesystem may need to modify
on-disk state to change mount options however that would get stuck since
the fs is frozen and thus deadlock would be created (you cannot thaw the
filesystem because the blocked process holds s_umount semaphore). Now also
thaw_super() relies on this as is behaves differently for RO and RW
filesystems.

> Would it be possible to relax this requirement depending on remount flags?

Yes, it would be possible although not trivial if you don't want to
introduce the above mentioned deadlock possibilities. And I don't see the
real need..

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[Amir Goldstein]

On Fri, Jan 20, 2017 at 10:27 AM, Jan Kara <jack@suse.cz> wrote:
> On Thu 19-01-17 21:03:12, Amir Goldstein wrote:
>> On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
>> I would be grateful if you guys could help me clarify something wrt
>> freezefs vs. remount.
>> I hope you may be able to enlighten me with just the information
>> provided in this email
>> without having to dive into overlayfs snapshot details.
>>
>> I implemented 'snapshot take' for overlayfs snapshots using a command like:
>> mount -o remount,snapshot=/snap/N /mnt
>>
>> The 'snapshot take' moment from overlayfs writers perspective is when
>> a single pointer
>> is modified via rcu_assign_pointer(ufs->__snapmnt, snapmnt):
>>
>> https://github.com/amir73il/linux/commit/f0d6759efb7a6281ecc55dcdf33837568f71d29a
>>
>> I chose to use the remount_fs() API because it is convenient and make
>> sense from user POV, but I could also use some IOCTL API for snapshot
>> take.
>
> So I don't see why remounting makes sense for making snapshots. Remounting
> is for manipulating mount options, not for instructing filesystem to make a
> snapshot. I think using ioctl(2) for manipulation of snapshots like btrfs
> does makes more sense (and it is also consistent with the existing
> precedens of btrfs).
>

Yes, its true, but the way overlay snapshots work is by mounting
and overlay with snapshot=<path> options which points to the
mount point where files are CoWed to, so the remount API makes
sense here.

Anyway, I think what I'll do is use the remount as setup phase
(prepare new snapshot target mount) and then use an ioctl
for the activation of the new snapshot.
That, or I could just check in ovl_freeze() if there is a new snapshot
setup and assign it, as snapshot take is always going to be
preceded by freeze anyway.


>> For consistency of snapshot from application POV, I need to do:
>> fsfreeze -f
>> snapshot take
>> fsfreeze -u
>>
>> But then I cannot use the remount_fs() API which requires that fs be unfrozen.
>>
>> Can you please explain why do_remount_sb() requires that fs be unfrozen?
>
> I think initially it was because we expected filesystem may need to modify
> on-disk state to change mount options however that would get stuck since
> the fs is frozen and thus deadlock would be created (you cannot thaw the
> filesystem because the blocked process holds s_umount semaphore). Now also
> thaw_super() relies on this as is behaves differently for RO and RW
> filesystems.
>
>> Would it be possible to relax this requirement depending on remount flags?
>
> Yes, it would be possible although not trivial if you don't want to
> introduce the above mentioned deadlock possibilities. And I don't see the
> real need..
>

I figures it was something like that. No I won't be able to justify making this
sort of change.

Thanks for explaining.

Re: [PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[Amir Goldstein]

On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Miklos,
>
> I implemented freeze/thaw of overlayfs, because I need it for
> overlay snapshots (CoW decision is made before mnt_wat_write(upper)
> and I need to serialize it with snapshot take).
>
> Not sure if there are other use cases for overlayfs freeze??
>
> Tested freeze stress with xfstest generic/068 generic/390.
>
> While staring at the code, I realized that syncfs(2) for overlayfs
> seems broken.  It looks like only inodes are synced and upper fs
> metadata is not being flushed, but I could be wrong.
>
> Tested sync sanity with -g quick (although no test calls syncfs directly).
> Tested the usual unionmount sanity over xfs and over tmpfs.
>
> I am not sure exactly how to write a test case to verify this alleged
> breakage?
>

Well, I have a smoking gun.
Wrote this xfs specific test, which checks xfs stats after syncfs/fsync:
https://github.com/amir73il/overlayfs/blob/master/tests/xfs_syncfs.sh

Good (on xfs 4.10.0-rc4):
# ./syncfs.sh /base/
before touch
xfs_log_force = 375
after touch
xfs_log_force = 375
after syncfs
xfs_log_force = 376
after fsync
xfs_log_force = 376
after fsync #2
xfs_log_force = 376

Bad (on overlayfs 4.10.0-rc4):
# ./syncfs.sh /mnt
before touch
xfs_log_force = 376
after touch
xfs_log_force = 376
after syncfs
xfs_log_force = 376
after fsync
xfs_log_force = 377
after fsync #2
xfs_log_force = 377

Overlayfs syncfs fails to flush the xfs log.

I'll see if I can put this test into xfstests.
I am going to need some sort of require_upper_fs_is_xfs
not sure id it already exists.

If anyone has suggestions how to write this test not xfs specific I
would be happy to hear.
Eryu?

Also, FYI, xfs_io -c syncfs is broken.
Going to send a fix patch soon.

Re: [PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[Eryu Guan]

On Fri, Jan 20, 2017 at 10:49:07AM +0200, Amir Goldstein wrote:
> On Thu, Jan 19, 2017 at 2:13 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> > Miklos,
> >
> > I implemented freeze/thaw of overlayfs, because I need it for
> > overlay snapshots (CoW decision is made before mnt_wat_write(upper)
> > and I need to serialize it with snapshot take).
> >
> > Not sure if there are other use cases for overlayfs freeze??
> >
> > Tested freeze stress with xfstest generic/068 generic/390.
> >
> > While staring at the code, I realized that syncfs(2) for overlayfs
> > seems broken.  It looks like only inodes are synced and upper fs
> > metadata is not being flushed, but I could be wrong.
> >
> > Tested sync sanity with -g quick (although no test calls syncfs directly).
> > Tested the usual unionmount sanity over xfs and over tmpfs.
> >
> > I am not sure exactly how to write a test case to verify this alleged
> > breakage?
> >
> 
> Well, I have a smoking gun.
> Wrote this xfs specific test, which checks xfs stats after syncfs/fsync:
> https://github.com/amir73il/overlayfs/blob/master/tests/xfs_syncfs.sh
> 
> Good (on xfs 4.10.0-rc4):
> # ./syncfs.sh /base/
> before touch
> xfs_log_force = 375
> after touch
> xfs_log_force = 375
> after syncfs
> xfs_log_force = 376
> after fsync
> xfs_log_force = 376
> after fsync #2
> xfs_log_force = 376
> 
> Bad (on overlayfs 4.10.0-rc4):
> # ./syncfs.sh /mnt
> before touch
> xfs_log_force = 376
> after touch
> xfs_log_force = 376
> after syncfs
> xfs_log_force = 376
> after fsync
> xfs_log_force = 377
> after fsync #2
> xfs_log_force = 377
> 
> Overlayfs syncfs fails to flush the xfs log.
> 
> I'll see if I can put this test into xfstests.
> I am going to need some sort of require_upper_fs_is_xfs
> not sure id it already exists.

There's no such _require rules in xfstests yet.

> 
> If anyone has suggestions how to write this test not xfs specific I
> would be happy to hear.
> Eryu?

Sorry, I have no idea right now..

Eryu
> 
> Also, FYI, xfs_io -c syncfs is broken.
> Going to send a fix patch soon.

Re: [PATCH 0/2] overlayfs: support freeze/thaw/syncfs

[Jan Kara]

Hi,

On Thu 19-01-17 14:13:26, Amir Goldstein wrote:
> I implemented freeze/thaw of overlayfs, because I need it for
> overlay snapshots (CoW decision is made before mnt_wat_write(upper)
> and I need to serialize it with snapshot take).
> 
> Not sure if there are other use cases for overlayfs freeze??
> 
> Tested freeze stress with xfstest generic/068 generic/390.
> 
> While staring at the code, I realized that syncfs(2) for overlayfs
> seems broken.  It looks like only inodes are synced and upper fs
> metadata is not being flushed, but I could be wrong.
> 
> Tested sync sanity with -g quick (although no test calls syncfs directly).
> Tested the usual unionmount sanity over xfs and over tmpfs.
> 
> I am not sure exactly how to write a test case to verify this alleged
> breakage?
> 
> Jan,
> 
> Can you please have a look?

So I don't really understand overlayfs but from my 10000 feet view your
changes make sense to me...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR