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

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

[Vivek Goyal]

Container folks are complaining that dnf/yum issues too many sync while
installing packages and this slows down the image build. Build
requirement is such that they don't care if a node goes down while
build was still going on. In that case, they will simply throw away
unfinished layer and start new build. So they don't care about syncing
intermediate state to the disk and hence don't want to pay the price
associated with sync.

So they are asking for mount options where they can disable sync on overlay
mount point.

They primarily seem to have two use cases.

- For building images, they will mount overlay with nosync and then sync
  upper layer after unmounting overlay and reuse upper as lower for next
  layer.

- For running containers, they don't seem to care about syncing upper
  layer because if node goes down, they will simply throw away upper
  layer and create a fresh one.

So this patch provides a mount option "volatile" which disables all forms
of sync. Now it is caller's responsibility to throw away upper if
system crashes or shuts down and start fresh.

With "volatile", I am seeing roughly 20% speed up in my VM where I am just
installing emacs in an image. Installation time drops from 31 seconds to
25 seconds when nosync option is used. This is for the case of building on top
of an image where all packages are already cached. That way I take
out the network operations latency out of the measurement.

Giuseppe is also looking to cut down on number of iops done on the
disk. He is complaining that often in cloud their VMs are throttled
if they cross the limit. This option can help them where they reduce
number of iops (by cutting down on frequent sync and writebacks).

Changes from v4:
- Dropped support for sync=fs (Miklos)
- Renamed "sync=off" to "volatile". (Miklos)

Changes from v3:
- Used only enums and dropped bit flags (Amir Goldstein)
- Dropped error when conflicting sync options provided. (Amir Goldstein)

Changes from v2:
- Added helper functions (Amir Goldstein)
- Used enums to keep sync state (Amir Goldstein)

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
---
 Documentation/filesystems/overlayfs.rst |  9 +++++++++
 fs/overlayfs/copy_up.c                  | 12 ++++++++----
 fs/overlayfs/file.c                     | 10 +++++++++-
 fs/overlayfs/ovl_entry.h                |  6 ++++++
 fs/overlayfs/readdir.c                  |  3 +++
 fs/overlayfs/super.c                    | 23 ++++++++++++++++++++---
 6 files changed, 55 insertions(+), 8 deletions(-)

diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
index fcda5d6ba9ac..d5fc5c94560e 100644
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -563,6 +563,15 @@ This verification may cause significant overhead in some cases.
 Note: the mount options index=off,nfs_export=on are conflicting for a
 read-write mount and will result in an error.
 
+Disable sync
+------------
+By default, overlay skips sync on files residing on a lower layer.  It
+is possible to skip sync operations for files on the upper layer as well
+with the "volatile" mount option.
+
+"volatile" mount option disables all forms of sync from overlay, including
+the one done at umount/remount. If system crashes or shuts down, user
+should throw away upper directory and start fresh.
 
 Testsuite
 ---------
diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
index 5e0cde85bd6b..ffb8334fe94d 100644
--- a/fs/overlayfs/copy_up.c
+++ b/fs/overlayfs/copy_up.c
@@ -128,7 +128,8 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
 	return error;
 }
 
-static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
+static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
+			    struct path *new, loff_t len)
 {
 	struct file *old_file;
 	struct file *new_file;
@@ -218,7 +219,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
 		len -= bytes;
 	}
 out:
-	if (!error)
+	if (!error && ovl_should_sync(ofs))
 		error = vfs_fsync(new_file, 0);
 	fput(new_file);
 out_fput:
@@ -484,6 +485,7 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
 
 static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 {
+	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	int err;
 
 	/*
@@ -499,7 +501,8 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
 		upperpath.dentry = temp;
 
 		ovl_path_lowerdata(c->dentry, &datapath);
-		err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
+		err = ovl_copy_up_data(ofs, &datapath, &upperpath,
+				       c->stat.size);
 		if (err)
 			return err;
 	}
@@ -784,6 +787,7 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
 /* Copy up data of an inode which was copied up metadata only in the past. */
 static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 {
+	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
 	struct path upperpath, datapath;
 	int err;
 	char *capability = NULL;
@@ -804,7 +808,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
 			goto out;
 	}
 
-	err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
+	err = ovl_copy_up_data(ofs, &datapath, &upperpath, c->stat.size);
 	if (err)
 		goto out_free;
 
diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
index 0d940e29d62b..3582c3ae819c 100644
--- a/fs/overlayfs/file.c
+++ b/fs/overlayfs/file.c
@@ -331,6 +331,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	struct fd real;
 	const struct cred *old_cred;
 	ssize_t ret;
+	int ifl = iocb->ki_flags;
 
 	if (!iov_iter_count(iter))
 		return 0;
@@ -346,11 +347,14 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 	if (ret)
 		goto out_unlock;
 
+	if (!ovl_should_sync(OVL_FS(inode->i_sb)))
+		ifl &= ~(IOCB_DSYNC | IOCB_SYNC);
+
 	old_cred = ovl_override_creds(file_inode(file)->i_sb);
 	if (is_sync_kiocb(iocb)) {
 		file_start_write(real.file);
 		ret = vfs_iter_write(real.file, iter, &iocb->ki_pos,
-				     ovl_iocb_to_rwf(iocb->ki_flags));
+				     ovl_iocb_to_rwf(ifl));
 		file_end_write(real.file);
 		/* Update size */
 		ovl_copyattr(ovl_inode_real(inode), inode);
@@ -370,6 +374,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
 		real.flags = 0;
 		aio_req->orig_iocb = iocb;
 		kiocb_clone(&aio_req->iocb, iocb, real.file);
+		aio_req->iocb.ki_flags = ifl;
 		aio_req->iocb.ki_complete = ovl_aio_rw_complete;
 		ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter);
 		if (ret != -EIOCBQUEUED)
@@ -433,6 +438,9 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
 	const struct cred *old_cred;
 	int ret;
 
+	if (!ovl_should_sync(OVL_FS(file_inode(file)->i_sb)))
+		return 0;
+
 	ret = ovl_real_fdget_meta(file, &real, !datasync);
 	if (ret)
 		return ret;
diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
index b429c80879ee..1b5a2094df8e 100644
--- a/fs/overlayfs/ovl_entry.h
+++ b/fs/overlayfs/ovl_entry.h
@@ -17,6 +17,7 @@ struct ovl_config {
 	bool nfs_export;
 	int xino;
 	bool metacopy;
+	bool ovl_volatile;
 };
 
 struct ovl_sb {
@@ -90,6 +91,11 @@ static inline struct ovl_fs *OVL_FS(struct super_block *sb)
 	return (struct ovl_fs *)sb->s_fs_info;
 }
 
+static inline bool ovl_should_sync(struct ovl_fs *ofs)
+{
+	return !ofs->config.ovl_volatile;
+}
+
 /* private information held for every overlayfs dentry */
 struct ovl_entry {
 	union {
diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
index 6918b98faeb6..2065c10ff0d1 100644
--- a/fs/overlayfs/readdir.c
+++ b/fs/overlayfs/readdir.c
@@ -863,6 +863,9 @@ static int ovl_dir_fsync(struct file *file, loff_t start, loff_t end,
 	if (!OVL_TYPE_UPPER(ovl_path_type(dentry)))
 		return 0;
 
+	if (!ovl_should_sync(OVL_FS(dentry->d_sb)))
+		return 0;
+
 	/*
 	 * Need to check if we started out being a lower dir, but got copied up
 	 */
diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
index 4b38141c2985..701fc4ad822c 100644
--- a/fs/overlayfs/super.c
+++ b/fs/overlayfs/super.c
@@ -264,6 +264,8 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
 	if (!ovl_upper_mnt(ofs))
 		return 0;
 
+	if (!ovl_should_sync(ofs))
+		return 0;
 	/*
 	 * Not called for sync(2) call or an emergency sync (SB_I_SKIP_SYNC).
 	 * All the super blocks will be iterated, including upper_sb.
@@ -362,6 +364,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
 	if (ofs->config.metacopy != ovl_metacopy_def)
 		seq_printf(m, ",metacopy=%s",
 			   ofs->config.metacopy ? "on" : "off");
+	if (ofs->config.ovl_volatile)
+		seq_printf(m, ",volatile");
 	return 0;
 }
 
@@ -376,9 +380,11 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
 
 	if (*flags & SB_RDONLY && !sb_rdonly(sb)) {
 		upper_sb = ovl_upper_mnt(ofs)->mnt_sb;
-		down_read(&upper_sb->s_umount);
-		ret = sync_filesystem(upper_sb);
-		up_read(&upper_sb->s_umount);
+		if (ovl_should_sync(ofs)) {
+			down_read(&upper_sb->s_umount);
+			ret = sync_filesystem(upper_sb);
+			up_read(&upper_sb->s_umount);
+		}
 	}
 
 	return ret;
@@ -411,6 +417,7 @@ enum {
 	OPT_XINO_AUTO,
 	OPT_METACOPY_ON,
 	OPT_METACOPY_OFF,
+	OPT_VOLATILE,
 	OPT_ERR,
 };
 
@@ -429,6 +436,7 @@ static const match_table_t ovl_tokens = {
 	{OPT_XINO_AUTO,			"xino=auto"},
 	{OPT_METACOPY_ON,		"metacopy=on"},
 	{OPT_METACOPY_OFF,		"metacopy=off"},
+	{OPT_VOLATILE,			"volatile"},
 	{OPT_ERR,			NULL}
 };
 
@@ -573,6 +581,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 			metacopy_opt = true;
 			break;
 
+		case OPT_VOLATILE:
+			config->ovl_volatile = true;
+			break;
+
 		default:
 			pr_err("unrecognized mount option \"%s\" or missing value\n",
 					p);
@@ -595,6 +607,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
 		config->index = false;
 	}
 
+	if (!config->upperdir && config->ovl_volatile) {
+		pr_info("option \"volatile\" is meaningless in a non-upper mount, ignoring it.\n");
+		config->ovl_volatile = false;
+	}
+
 	err = ovl_parse_redirect_mode(config, config->redirect_mode);
 	if (err)
 		return err;
-- 
2.25.4

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

[Giuseppe Scrivano]

Vivek Goyal <vgoyal@redhat.com> writes:

> Container folks are complaining that dnf/yum issues too many sync while
> installing packages and this slows down the image build. Build
> requirement is such that they don't care if a node goes down while
> build was still going on. In that case, they will simply throw away
> unfinished layer and start new build. So they don't care about syncing
> intermediate state to the disk and hence don't want to pay the price
> associated with sync.
>
> So they are asking for mount options where they can disable sync on overlay
> mount point.
>
> They primarily seem to have two use cases.
>
> - For building images, they will mount overlay with nosync and then sync
>   upper layer after unmounting overlay and reuse upper as lower for next
>   layer.
>
> - For running containers, they don't seem to care about syncing upper
>   layer because if node goes down, they will simply throw away upper
>   layer and create a fresh one.
>
> So this patch provides a mount option "volatile" which disables all forms
> of sync. Now it is caller's responsibility to throw away upper if
> system crashes or shuts down and start fresh.
>
> With "volatile", I am seeing roughly 20% speed up in my VM where I am just
> installing emacs in an image. Installation time drops from 31 seconds to
> 25 seconds when nosync option is used. This is for the case of building on top
> of an image where all packages are already cached. That way I take
> out the network operations latency out of the measurement.
>
> Giuseppe is also looking to cut down on number of iops done on the
> disk. He is complaining that often in cloud their VMs are throttled
> if they cross the limit. This option can help them where they reduce
> number of iops (by cutting down on frequent sync and writebacks).
>
> Changes from v4:
> - Dropped support for sync=fs (Miklos)
> - Renamed "sync=off" to "volatile". (Miklos)
>
> Changes from v3:
> - Used only enums and dropped bit flags (Amir Goldstein)
> - Dropped error when conflicting sync options provided. (Amir Goldstein)
>
> Changes from v2:
> - Added helper functions (Amir Goldstein)
> - Used enums to keep sync state (Amir Goldstein)
>
> Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
> Signed-off-by: Miklos Szeredi <mszeredi@redhat.com>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> ---
>  Documentation/filesystems/overlayfs.rst |  9 +++++++++
>  fs/overlayfs/copy_up.c                  | 12 ++++++++----
>  fs/overlayfs/file.c                     | 10 +++++++++-
>  fs/overlayfs/ovl_entry.h                |  6 ++++++
>  fs/overlayfs/readdir.c                  |  3 +++
>  fs/overlayfs/super.c                    | 23 ++++++++++++++++++++---
>  6 files changed, 55 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index fcda5d6ba9ac..d5fc5c94560e 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -563,6 +563,15 @@ This verification may cause significant overhead in some cases.
>  Note: the mount options index=off,nfs_export=on are conflicting for a
>  read-write mount and will result in an error.
>  
> +Disable sync
> +------------
> +By default, overlay skips sync on files residing on a lower layer.  It
> +is possible to skip sync operations for files on the upper layer as well
> +with the "volatile" mount option.
> +
> +"volatile" mount option disables all forms of sync from overlay, including
> +the one done at umount/remount. If system crashes or shuts down, user
> +should throw away upper directory and start fresh.
>  
>  Testsuite
>  ---------
> diff --git a/fs/overlayfs/copy_up.c b/fs/overlayfs/copy_up.c
> index 5e0cde85bd6b..ffb8334fe94d 100644
> --- a/fs/overlayfs/copy_up.c
> +++ b/fs/overlayfs/copy_up.c
> @@ -128,7 +128,8 @@ int ovl_copy_xattr(struct dentry *old, struct dentry *new)
>  	return error;
>  }
>  
> -static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
> +static int ovl_copy_up_data(struct ovl_fs *ofs, struct path *old,
> +			    struct path *new, loff_t len)
>  {
>  	struct file *old_file;
>  	struct file *new_file;
> @@ -218,7 +219,7 @@ static int ovl_copy_up_data(struct path *old, struct path *new, loff_t len)
>  		len -= bytes;
>  	}
>  out:
> -	if (!error)
> +	if (!error && ovl_should_sync(ofs))
>  		error = vfs_fsync(new_file, 0);
>  	fput(new_file);
>  out_fput:
> @@ -484,6 +485,7 @@ static int ovl_link_up(struct ovl_copy_up_ctx *c)
>  
>  static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>  {
> +	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
>  	int err;
>  
>  	/*
> @@ -499,7 +501,8 @@ static int ovl_copy_up_inode(struct ovl_copy_up_ctx *c, struct dentry *temp)
>  		upperpath.dentry = temp;
>  
>  		ovl_path_lowerdata(c->dentry, &datapath);
> -		err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
> +		err = ovl_copy_up_data(ofs, &datapath, &upperpath,
> +				       c->stat.size);
>  		if (err)
>  			return err;
>  	}
> @@ -784,6 +787,7 @@ static bool ovl_need_meta_copy_up(struct dentry *dentry, umode_t mode,
>  /* Copy up data of an inode which was copied up metadata only in the past. */
>  static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>  {
> +	struct ovl_fs *ofs = OVL_FS(c->dentry->d_sb);
>  	struct path upperpath, datapath;
>  	int err;
>  	char *capability = NULL;
> @@ -804,7 +808,7 @@ static int ovl_copy_up_meta_inode_data(struct ovl_copy_up_ctx *c)
>  			goto out;
>  	}
>  
> -	err = ovl_copy_up_data(&datapath, &upperpath, c->stat.size);
> +	err = ovl_copy_up_data(ofs, &datapath, &upperpath, c->stat.size);
>  	if (err)
>  		goto out_free;
>  
> diff --git a/fs/overlayfs/file.c b/fs/overlayfs/file.c
> index 0d940e29d62b..3582c3ae819c 100644
> --- a/fs/overlayfs/file.c
> +++ b/fs/overlayfs/file.c
> @@ -331,6 +331,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>  	struct fd real;
>  	const struct cred *old_cred;
>  	ssize_t ret;
> +	int ifl = iocb->ki_flags;
>  
>  	if (!iov_iter_count(iter))
>  		return 0;
> @@ -346,11 +347,14 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>  	if (ret)
>  		goto out_unlock;
>  
> +	if (!ovl_should_sync(OVL_FS(inode->i_sb)))
> +		ifl &= ~(IOCB_DSYNC | IOCB_SYNC);
> +
>  	old_cred = ovl_override_creds(file_inode(file)->i_sb);
>  	if (is_sync_kiocb(iocb)) {
>  		file_start_write(real.file);
>  		ret = vfs_iter_write(real.file, iter, &iocb->ki_pos,
> -				     ovl_iocb_to_rwf(iocb->ki_flags));
> +				     ovl_iocb_to_rwf(ifl));
>  		file_end_write(real.file);
>  		/* Update size */
>  		ovl_copyattr(ovl_inode_real(inode), inode);
> @@ -370,6 +374,7 @@ static ssize_t ovl_write_iter(struct kiocb *iocb, struct iov_iter *iter)
>  		real.flags = 0;
>  		aio_req->orig_iocb = iocb;
>  		kiocb_clone(&aio_req->iocb, iocb, real.file);
> +		aio_req->iocb.ki_flags = ifl;
>  		aio_req->iocb.ki_complete = ovl_aio_rw_complete;
>  		ret = vfs_iocb_iter_write(real.file, &aio_req->iocb, iter);
>  		if (ret != -EIOCBQUEUED)
> @@ -433,6 +438,9 @@ static int ovl_fsync(struct file *file, loff_t start, loff_t end, int datasync)
>  	const struct cred *old_cred;
>  	int ret;
>  
> +	if (!ovl_should_sync(OVL_FS(file_inode(file)->i_sb)))
> +		return 0;
> +
>  	ret = ovl_real_fdget_meta(file, &real, !datasync);
>  	if (ret)
>  		return ret;
> diff --git a/fs/overlayfs/ovl_entry.h b/fs/overlayfs/ovl_entry.h
> index b429c80879ee..1b5a2094df8e 100644
> --- a/fs/overlayfs/ovl_entry.h
> +++ b/fs/overlayfs/ovl_entry.h
> @@ -17,6 +17,7 @@ struct ovl_config {
>  	bool nfs_export;
>  	int xino;
>  	bool metacopy;
> +	bool ovl_volatile;
>  };
>  
>  struct ovl_sb {
> @@ -90,6 +91,11 @@ static inline struct ovl_fs *OVL_FS(struct super_block *sb)
>  	return (struct ovl_fs *)sb->s_fs_info;
>  }
>  
> +static inline bool ovl_should_sync(struct ovl_fs *ofs)
> +{
> +	return !ofs->config.ovl_volatile;
> +}
> +
>  /* private information held for every overlayfs dentry */
>  struct ovl_entry {
>  	union {
> diff --git a/fs/overlayfs/readdir.c b/fs/overlayfs/readdir.c
> index 6918b98faeb6..2065c10ff0d1 100644
> --- a/fs/overlayfs/readdir.c
> +++ b/fs/overlayfs/readdir.c
> @@ -863,6 +863,9 @@ static int ovl_dir_fsync(struct file *file, loff_t start, loff_t end,
>  	if (!OVL_TYPE_UPPER(ovl_path_type(dentry)))
>  		return 0;
>  
> +	if (!ovl_should_sync(OVL_FS(dentry->d_sb)))
> +		return 0;
> +
>  	/*
>  	 * Need to check if we started out being a lower dir, but got copied up
>  	 */
> diff --git a/fs/overlayfs/super.c b/fs/overlayfs/super.c
> index 4b38141c2985..701fc4ad822c 100644
> --- a/fs/overlayfs/super.c
> +++ b/fs/overlayfs/super.c
> @@ -264,6 +264,8 @@ static int ovl_sync_fs(struct super_block *sb, int wait)
>  	if (!ovl_upper_mnt(ofs))
>  		return 0;
>  
> +	if (!ovl_should_sync(ofs))
> +		return 0;
>  	/*
>  	 * Not called for sync(2) call or an emergency sync (SB_I_SKIP_SYNC).
>  	 * All the super blocks will be iterated, including upper_sb.
> @@ -362,6 +364,8 @@ static int ovl_show_options(struct seq_file *m, struct dentry *dentry)
>  	if (ofs->config.metacopy != ovl_metacopy_def)
>  		seq_printf(m, ",metacopy=%s",
>  			   ofs->config.metacopy ? "on" : "off");
> +	if (ofs->config.ovl_volatile)
> +		seq_printf(m, ",volatile");
>  	return 0;
>  }
>  
> @@ -376,9 +380,11 @@ static int ovl_remount(struct super_block *sb, int *flags, char *data)
>  
>  	if (*flags & SB_RDONLY && !sb_rdonly(sb)) {
>  		upper_sb = ovl_upper_mnt(ofs)->mnt_sb;
> -		down_read(&upper_sb->s_umount);
> -		ret = sync_filesystem(upper_sb);
> -		up_read(&upper_sb->s_umount);
> +		if (ovl_should_sync(ofs)) {
> +			down_read(&upper_sb->s_umount);
> +			ret = sync_filesystem(upper_sb);
> +			up_read(&upper_sb->s_umount);
> +		}
>  	}
>  
>  	return ret;
> @@ -411,6 +417,7 @@ enum {
>  	OPT_XINO_AUTO,
>  	OPT_METACOPY_ON,
>  	OPT_METACOPY_OFF,
> +	OPT_VOLATILE,
>  	OPT_ERR,
>  };
>  
> @@ -429,6 +436,7 @@ static const match_table_t ovl_tokens = {
>  	{OPT_XINO_AUTO,			"xino=auto"},
>  	{OPT_METACOPY_ON,		"metacopy=on"},
>  	{OPT_METACOPY_OFF,		"metacopy=off"},
> +	{OPT_VOLATILE,			"volatile"},
>  	{OPT_ERR,			NULL}
>  };
>  
> @@ -573,6 +581,10 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  			metacopy_opt = true;
>  			break;
>  
> +		case OPT_VOLATILE:
> +			config->ovl_volatile = true;
> +			break;
> +
>  		default:
>  			pr_err("unrecognized mount option \"%s\" or missing value\n",
>  					p);
> @@ -595,6 +607,11 @@ static int ovl_parse_opt(char *opt, struct ovl_config *config)
>  		config->index = false;
>  	}
>  
> +	if (!config->upperdir && config->ovl_volatile) {
> +		pr_info("option \"volatile\" is meaningless in a non-upper mount, ignoring it.\n");
> +		config->ovl_volatile = false;
> +	}
> +
>  	err = ovl_parse_redirect_mode(config, config->redirect_mode);
>  	if (err)
>  		return err;

Ping.

Is there anything holding this patch?

Thanks,
Giuseppe

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

[Miklos Szeredi]

On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>
> Vivek Goyal <vgoyal@redhat.com> writes:
>
> > Container folks are complaining that dnf/yum issues too many sync while
> > installing packages and this slows down the image build. Build
> > requirement is such that they don't care if a node goes down while
> > build was still going on. In that case, they will simply throw away
> > unfinished layer and start new build. So they don't care about syncing
> > intermediate state to the disk and hence don't want to pay the price
> > associated with sync.
> >

[...]

> Ping.
>
> Is there anything holding this patch?

Not sure what happened with protection against mounting a volatile
overlay twice, I don't see that in the patch.

With that added, I don't see anything else holding it back.

Thanks,
Miklos

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

[Amir Goldstein]

On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >
> > Vivek Goyal <vgoyal@redhat.com> writes:
> >
> > > Container folks are complaining that dnf/yum issues too many sync while
> > > installing packages and this slows down the image build. Build
> > > requirement is such that they don't care if a node goes down while
> > > build was still going on. In that case, they will simply throw away
> > > unfinished layer and start new build. So they don't care about syncing
> > > intermediate state to the disk and hence don't want to pay the price
> > > associated with sync.
> > >
>
> [...]
>
> > Ping.
> >
> > Is there anything holding this patch?
>
> Not sure what happened with protection against mounting a volatile
> overlay twice, I don't see that in the patch.

Do you mean protection only for new kernels or old kernels as well?

The latter can be achieved by using $workdir/volatile/ as upperdir
instead of $upperdir.
Or maybe even use $workdir/work/incompat/volatile/upper, so if older
kernel tries to re-use that $workdir, it will fail to mount rw with error:

  overlayfs: cleanup of 'incompat/volatile' failed (-39)

If we agree to that, then upperdir= should not be provided at all when
specifying "volatile".

Thanks,
Amir.

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

[Miklos Szeredi]

On Mon, Aug 24, 2020 at 12:59 PM Amir Goldstein <amir73il@gmail.com> wrote:

 >
> > Not sure what happened with protection against mounting a volatile
> > overlay twice, I don't see that in the patch.
>
> Do you mean protection only for new kernels or old kernels as well?
>
> The latter can be achieved by using $workdir/volatile/ as upperdir
> instead of $upperdir.
> Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> kernel tries to re-use that $workdir, it will fail to mount rw with error:
>
>   overlayfs: cleanup of 'incompat/volatile' failed (-39)
>
> If we agree to that, then upperdir= should not be provided at all when
> specifying "volatile".

Good point about failing with old kernels.

Yeah, using workdir for upper makes sense due to the volatile nature
of the upper layer.

Thanks,
Miklos


>
> Thanks,
> Amir.

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

[Giuseppe Scrivano]

Hi Amir,

Amir Goldstein <amir73il@gmail.com> writes:

> On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>
>> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>> >
>> > Vivek Goyal <vgoyal@redhat.com> writes:
>> >
>> > > Container folks are complaining that dnf/yum issues too many sync while
>> > > installing packages and this slows down the image build. Build
>> > > requirement is such that they don't care if a node goes down while
>> > > build was still going on. In that case, they will simply throw away
>> > > unfinished layer and start new build. So they don't care about syncing
>> > > intermediate state to the disk and hence don't want to pay the price
>> > > associated with sync.
>> > >
>>
>> [...]
>>
>> > Ping.
>> >
>> > Is there anything holding this patch?
>>
>> Not sure what happened with protection against mounting a volatile
>> overlay twice, I don't see that in the patch.
>
> Do you mean protection only for new kernels or old kernels as well?
>
> The latter can be achieved by using $workdir/volatile/ as upperdir
> instead of $upperdir.
> Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> kernel tries to re-use that $workdir, it will fail to mount rw with error:
>
>   overlayfs: cleanup of 'incompat/volatile' failed (-39)
>
> If we agree to that, then upperdir= should not be provided at all when
> specifying "volatile".

in this case, what does a program need to do to remount the overlay more
than once?  Is it enough to just delete a file? 

Thanks,
Giuseppe

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

[Amir Goldstein]

On Mon, Aug 24, 2020 at 2:39 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>
> Hi Amir,
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >>
> >> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >> >
> >> > Vivek Goyal <vgoyal@redhat.com> writes:
> >> >
> >> > > Container folks are complaining that dnf/yum issues too many sync while
> >> > > installing packages and this slows down the image build. Build
> >> > > requirement is such that they don't care if a node goes down while
> >> > > build was still going on. In that case, they will simply throw away
> >> > > unfinished layer and start new build. So they don't care about syncing
> >> > > intermediate state to the disk and hence don't want to pay the price
> >> > > associated with sync.
> >> > >
> >>
> >> [...]
> >>
> >> > Ping.
> >> >
> >> > Is there anything holding this patch?
> >>
> >> Not sure what happened with protection against mounting a volatile
> >> overlay twice, I don't see that in the patch.
> >
> > Do you mean protection only for new kernels or old kernels as well?
> >
> > The latter can be achieved by using $workdir/volatile/ as upperdir
> > instead of $upperdir.
> > Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> > kernel tries to re-use that $workdir, it will fail to mount rw with error:
> >
> >   overlayfs: cleanup of 'incompat/volatile' failed (-39)
> >
> > If we agree to that, then upperdir= should not be provided at all when
> > specifying "volatile".
>
> in this case, what does a program need to do to remount the overlay more
> than once?  Is it enough to just delete a file?
>

Do you mean re-mount while forgetting all changes to previous "volatile"
mount?

The "workdir" should be re-created from scratch.
IOW, rm -rf $workdir/volatile or rm -rf $workdir/incomapt depending
on which one of my suggested alternatives is chosen.

Thanks,
Amir.

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

[Giuseppe Scrivano]

Amir Goldstein <amir73il@gmail.com> writes:

> On Mon, Aug 24, 2020 at 2:39 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>>
>> Hi Amir,
>>
>> Amir Goldstein <amir73il@gmail.com> writes:
>>
>> > On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
>> >>
>> >> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>> >> >
>> >> > Vivek Goyal <vgoyal@redhat.com> writes:
>> >> >
>> >> > > Container folks are complaining that dnf/yum issues too many sync while
>> >> > > installing packages and this slows down the image build. Build
>> >> > > requirement is such that they don't care if a node goes down while
>> >> > > build was still going on. In that case, they will simply throw away
>> >> > > unfinished layer and start new build. So they don't care about syncing
>> >> > > intermediate state to the disk and hence don't want to pay the price
>> >> > > associated with sync.
>> >> > >
>> >>
>> >> [...]
>> >>
>> >> > Ping.
>> >> >
>> >> > Is there anything holding this patch?
>> >>
>> >> Not sure what happened with protection against mounting a volatile
>> >> overlay twice, I don't see that in the patch.
>> >
>> > Do you mean protection only for new kernels or old kernels as well?
>> >
>> > The latter can be achieved by using $workdir/volatile/ as upperdir
>> > instead of $upperdir.
>> > Or maybe even use $workdir/work/incompat/volatile/upper, so if older
>> > kernel tries to re-use that $workdir, it will fail to mount rw with error:
>> >
>> >   overlayfs: cleanup of 'incompat/volatile' failed (-39)
>> >
>> > If we agree to that, then upperdir= should not be provided at all when
>> > specifying "volatile".
>>
>> in this case, what does a program need to do to remount the overlay more
>> than once?  Is it enough to just delete a file?
>>
>
> Do you mean re-mount while forgetting all changes to previous "volatile"
> mount?

no, without forgetting them.
The original idea was to have a way to disable any sync operation in the
overlay file system and let the upper layers handle it.  IOW, mount
volatile overlay+umount overlay+syncfs upper dir must still be
considered safe.
If we want to make it safer and disallow remounting the same
workdir+upperdir by default when "volatile" is used, that is fine; but I
think there should still be a way to say "I know what I am doing, just
remount it".

Regards,
Giuseppe

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

[Miklos Szeredi]

On Mon, Aug 24, 2020 at 3:02 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
>
> Amir Goldstein <amir73il@gmail.com> writes:
>
> > On Mon, Aug 24, 2020 at 2:39 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >>
> >> Hi Amir,
> >>
> >> Amir Goldstein <amir73il@gmail.com> writes:
> >>
> >> > On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >> >>
> >> >> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >> >> >
> >> >> > Vivek Goyal <vgoyal@redhat.com> writes:
> >> >> >
> >> >> > > Container folks are complaining that dnf/yum issues too many sync while
> >> >> > > installing packages and this slows down the image build. Build
> >> >> > > requirement is such that they don't care if a node goes down while
> >> >> > > build was still going on. In that case, they will simply throw away
> >> >> > > unfinished layer and start new build. So they don't care about syncing
> >> >> > > intermediate state to the disk and hence don't want to pay the price
> >> >> > > associated with sync.
> >> >> > >
> >> >>
> >> >> [...]
> >> >>
> >> >> > Ping.
> >> >> >
> >> >> > Is there anything holding this patch?
> >> >>
> >> >> Not sure what happened with protection against mounting a volatile
> >> >> overlay twice, I don't see that in the patch.
> >> >
> >> > Do you mean protection only for new kernels or old kernels as well?
> >> >
> >> > The latter can be achieved by using $workdir/volatile/ as upperdir
> >> > instead of $upperdir.
> >> > Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> >> > kernel tries to re-use that $workdir, it will fail to mount rw with error:
> >> >
> >> >   overlayfs: cleanup of 'incompat/volatile' failed (-39)
> >> >
> >> > If we agree to that, then upperdir= should not be provided at all when
> >> > specifying "volatile".
> >>
> >> in this case, what does a program need to do to remount the overlay more
> >> than once?  Is it enough to just delete a file?
> >>
> >
> > Do you mean re-mount while forgetting all changes to previous "volatile"
> > mount?
>
> no, without forgetting them.
> The original idea was to have a way to disable any sync operation in the
> overlay file system and let the upper layers handle it.  IOW, mount
> volatile overlay+umount overlay+syncfs upper dir must still be
> considered safe.
> If we want to make it safer and disallow remounting the same
> workdir+upperdir by default when "volatile" is used, that is fine; but I
> think there should still be a way to say "I know what I am doing, just
> remount it".

Indeed.  "Volatile" doesn't mean you can't use the data, just that the
data may be lost completely in case of a crash (tmpfs analogue).

Maybe just stick

  $(workdir)/work/incompat/volatile/donotremove

in there to prevent misuse.

Thanks,
Miklos

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

[Vivek Goyal]

On Mon, Aug 24, 2020 at 01:59:41PM +0300, Amir Goldstein wrote:
> On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >
> > On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > >
> > > Vivek Goyal <vgoyal@redhat.com> writes:
> > >
> > > > Container folks are complaining that dnf/yum issues too many sync while
> > > > installing packages and this slows down the image build. Build
> > > > requirement is such that they don't care if a node goes down while
> > > > build was still going on. In that case, they will simply throw away
> > > > unfinished layer and start new build. So they don't care about syncing
> > > > intermediate state to the disk and hence don't want to pay the price
> > > > associated with sync.
> > > >
> >
> > [...]
> >
> > > Ping.
> > >
> > > Is there anything holding this patch?
> >
> > Not sure what happened with protection against mounting a volatile
> > overlay twice, I don't see that in the patch.
> 
> Do you mean protection only for new kernels or old kernels as well?
> 
> The latter can be achieved by using $workdir/volatile/ as upperdir
> instead of $upperdir.
> Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> kernel tries to re-use that $workdir, it will fail to mount rw with error:
> 
>   overlayfs: cleanup of 'incompat/volatile' failed (-39)
> 
> If we agree to that, then upperdir= should not be provided at all when
> specifying "volatile".

If we keep volatile inside workdir, then we fail work and upperdir
being separate subtree checks. And I suspect that all that trap
magic will trigger too.

I think for image building use case, tools to have access to volatile
directory. So that they can persist it, rename it and use it as lower
layer for next layer build. That means we will have to document it
and let users access and rename $workdir/work/incompat/volatile/ or
$workdir/work/volatile.

Once Miklos has suggested to drop a file in workdir say $workdir/volatile
And next remount will refuse to mount that overlay instance if
$workdir/volatile is present. With this approach work/ and upper/
are in separate dir subtrees.

And user will be forced to remove work/ and upper/ if previous instance
was mounted with "volatile".

I am not too worried about protection against older kernels because if
system has been setup to boot into a new kernel, it will boot into
new kernel again. (Until and unless somebody forces it to go back to
old kernel). But if you think providing protection against old kernels
is important, we could create volatile in $workdir/work/dir1/dir2/volatile
instead.

/me is wondering why I don't get error with $workdir/work/dir1/volatile
but I do with $workdir/work/dir1/dir2/volatile. IOW, why in first
case removal of dir1 was successful despite the fact it is non-empty.

Thanks
Vivek

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

[Vivek Goyal]

On Mon, Aug 24, 2020 at 03:20:20PM +0200, Miklos Szeredi wrote:
> On Mon, Aug 24, 2020 at 3:02 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> >
> > Amir Goldstein <amir73il@gmail.com> writes:
> >
> > > On Mon, Aug 24, 2020 at 2:39 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > >>
> > >> Hi Amir,
> > >>
> > >> Amir Goldstein <amir73il@gmail.com> writes:
> > >>
> > >> > On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > >> >>
> > >> >> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > >> >> >
> > >> >> > Vivek Goyal <vgoyal@redhat.com> writes:
> > >> >> >
> > >> >> > > Container folks are complaining that dnf/yum issues too many sync while
> > >> >> > > installing packages and this slows down the image build. Build
> > >> >> > > requirement is such that they don't care if a node goes down while
> > >> >> > > build was still going on. In that case, they will simply throw away
> > >> >> > > unfinished layer and start new build. So they don't care about syncing
> > >> >> > > intermediate state to the disk and hence don't want to pay the price
> > >> >> > > associated with sync.
> > >> >> > >
> > >> >>
> > >> >> [...]
> > >> >>
> > >> >> > Ping.
> > >> >> >
> > >> >> > Is there anything holding this patch?
> > >> >>
> > >> >> Not sure what happened with protection against mounting a volatile
> > >> >> overlay twice, I don't see that in the patch.
> > >> >
> > >> > Do you mean protection only for new kernels or old kernels as well?
> > >> >
> > >> > The latter can be achieved by using $workdir/volatile/ as upperdir
> > >> > instead of $upperdir.
> > >> > Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> > >> > kernel tries to re-use that $workdir, it will fail to mount rw with error:
> > >> >
> > >> >   overlayfs: cleanup of 'incompat/volatile' failed (-39)
> > >> >
> > >> > If we agree to that, then upperdir= should not be provided at all when
> > >> > specifying "volatile".
> > >>
> > >> in this case, what does a program need to do to remount the overlay more
> > >> than once?  Is it enough to just delete a file?
> > >>
> > >
> > > Do you mean re-mount while forgetting all changes to previous "volatile"
> > > mount?
> >
> > no, without forgetting them.
> > The original idea was to have a way to disable any sync operation in the
> > overlay file system and let the upper layers handle it.  IOW, mount
> > volatile overlay+umount overlay+syncfs upper dir must still be
> > considered safe.
> > If we want to make it safer and disallow remounting the same
> > workdir+upperdir by default when "volatile" is used, that is fine; but I
> > think there should still be a way to say "I know what I am doing, just
> > remount it".
> 
> Indeed.  "Volatile" doesn't mean you can't use the data, just that the
> data may be lost completely in case of a crash (tmpfs analogue).
> 
> Maybe just stick
> 
>   $(workdir)/work/incompat/volatile/donotremove
> 
> in there to prevent misuse.

So we ask users to remove "$(workdir)/work/incompat/volatile/donotremove"
if they want to remount with with same upper/ and work/? (Presumably
after syncing upper/).

Thanks
Vivek

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

[Miklos Szeredi]

On Mon, Aug 24, 2020 at 3:39 PM Vivek Goyal <vgoyal@redhat.com> wrote:

> /me is wondering why I don't get error with $workdir/work/dir1/volatile
> but I do with $workdir/work/dir1/dir2/volatile. IOW, why in first
> case removal of dir1 was successful despite the fact it is non-empty.

It cares for the case when a directory full of whiteouts is echanged
with RENAME_EXCHANGE to an empty opaque dir created under
$workdir/work.

Strictly speaking it wouldn't need to be able to remove directories
under dir1, but it's just reusing the generic cleanup routine, so
everything is cleaned up upto the second level.

Thanks,
Miklos

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

[Amir Goldstein]

On Mon, Aug 24, 2020 at 4:51 PM Vivek Goyal <vgoyal@redhat.com> wrote:
>
> On Mon, Aug 24, 2020 at 03:20:20PM +0200, Miklos Szeredi wrote:
> > On Mon, Aug 24, 2020 at 3:02 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > >
> > > Amir Goldstein <amir73il@gmail.com> writes:
> > >
> > > > On Mon, Aug 24, 2020 at 2:39 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > >>
> > > >> Hi Amir,
> > > >>
> > > >> Amir Goldstein <amir73il@gmail.com> writes:
> > > >>
> > > >> > On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > >> >>
> > > >> >> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > >> >> >
> > > >> >> > Vivek Goyal <vgoyal@redhat.com> writes:
> > > >> >> >
> > > >> >> > > Container folks are complaining that dnf/yum issues too many sync while
> > > >> >> > > installing packages and this slows down the image build. Build
> > > >> >> > > requirement is such that they don't care if a node goes down while
> > > >> >> > > build was still going on. In that case, they will simply throw away
> > > >> >> > > unfinished layer and start new build. So they don't care about syncing
> > > >> >> > > intermediate state to the disk and hence don't want to pay the price
> > > >> >> > > associated with sync.
> > > >> >> > >
> > > >> >>
> > > >> >> [...]
> > > >> >>
> > > >> >> > Ping.
> > > >> >> >
> > > >> >> > Is there anything holding this patch?
> > > >> >>
> > > >> >> Not sure what happened with protection against mounting a volatile
> > > >> >> overlay twice, I don't see that in the patch.
> > > >> >
> > > >> > Do you mean protection only for new kernels or old kernels as well?
> > > >> >
> > > >> > The latter can be achieved by using $workdir/volatile/ as upperdir
> > > >> > instead of $upperdir.
> > > >> > Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> > > >> > kernel tries to re-use that $workdir, it will fail to mount rw with error:
> > > >> >
> > > >> >   overlayfs: cleanup of 'incompat/volatile' failed (-39)
> > > >> >
> > > >> > If we agree to that, then upperdir= should not be provided at all when
> > > >> > specifying "volatile".
> > > >>
> > > >> in this case, what does a program need to do to remount the overlay more
> > > >> than once?  Is it enough to just delete a file?
> > > >>
> > > >
> > > > Do you mean re-mount while forgetting all changes to previous "volatile"
> > > > mount?
> > >
> > > no, without forgetting them.
> > > The original idea was to have a way to disable any sync operation in the
> > > overlay file system and let the upper layers handle it.  IOW, mount
> > > volatile overlay+umount overlay+syncfs upper dir must still be
> > > considered safe.
> > > If we want to make it safer and disallow remounting the same
> > > workdir+upperdir by default when "volatile" is used, that is fine; but I
> > > think there should still be a way to say "I know what I am doing, just
> > > remount it".
> >
> > Indeed.  "Volatile" doesn't mean you can't use the data, just that the
> > data may be lost completely in case of a crash (tmpfs analogue).
> >
> > Maybe just stick
> >
> >   $(workdir)/work/incompat/volatile/donotremove
> >
> > in there to prevent misuse.
>
> So we ask users to remove "$(workdir)/work/incompat/volatile/donotremove"
> if they want to remount with with same upper/ and work/? (Presumably
> after syncing upper/).
>

Sounds right.
Just don't rely on the workdir cleanup error yes?
That protection is for old kernels and it falls back to r/o mount.
New kernel should of course recognise $(workdir)/work/incompat/volatile
fail to mount and explicitly error about unclean "volatile" unmount and
maybe give a hint how to fix it in kernel log.

Thanks,
Amir.

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

[Vivek Goyal]

On Mon, Aug 24, 2020 at 11:53:30PM +0300, Amir Goldstein wrote:
> On Mon, Aug 24, 2020 at 4:51 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Mon, Aug 24, 2020 at 03:20:20PM +0200, Miklos Szeredi wrote:
> > > On Mon, Aug 24, 2020 at 3:02 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > >
> > > > Amir Goldstein <amir73il@gmail.com> writes:
> > > >
> > > > > On Mon, Aug 24, 2020 at 2:39 PM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > > >>
> > > > >> Hi Amir,
> > > > >>
> > > > >> Amir Goldstein <amir73il@gmail.com> writes:
> > > > >>
> > > > >> > On Mon, Aug 24, 2020 at 11:15 AM Miklos Szeredi <miklos@szeredi.hu> wrote:
> > > > >> >>
> > > > >> >> On Sat, Aug 22, 2020 at 11:27 AM Giuseppe Scrivano <gscrivan@redhat.com> wrote:
> > > > >> >> >
> > > > >> >> > Vivek Goyal <vgoyal@redhat.com> writes:
> > > > >> >> >
> > > > >> >> > > Container folks are complaining that dnf/yum issues too many sync while
> > > > >> >> > > installing packages and this slows down the image build. Build
> > > > >> >> > > requirement is such that they don't care if a node goes down while
> > > > >> >> > > build was still going on. In that case, they will simply throw away
> > > > >> >> > > unfinished layer and start new build. So they don't care about syncing
> > > > >> >> > > intermediate state to the disk and hence don't want to pay the price
> > > > >> >> > > associated with sync.
> > > > >> >> > >
> > > > >> >>
> > > > >> >> [...]
> > > > >> >>
> > > > >> >> > Ping.
> > > > >> >> >
> > > > >> >> > Is there anything holding this patch?
> > > > >> >>
> > > > >> >> Not sure what happened with protection against mounting a volatile
> > > > >> >> overlay twice, I don't see that in the patch.
> > > > >> >
> > > > >> > Do you mean protection only for new kernels or old kernels as well?
> > > > >> >
> > > > >> > The latter can be achieved by using $workdir/volatile/ as upperdir
> > > > >> > instead of $upperdir.
> > > > >> > Or maybe even use $workdir/work/incompat/volatile/upper, so if older
> > > > >> > kernel tries to re-use that $workdir, it will fail to mount rw with error:
> > > > >> >
> > > > >> >   overlayfs: cleanup of 'incompat/volatile' failed (-39)
> > > > >> >
> > > > >> > If we agree to that, then upperdir= should not be provided at all when
> > > > >> > specifying "volatile".
> > > > >>
> > > > >> in this case, what does a program need to do to remount the overlay more
> > > > >> than once?  Is it enough to just delete a file?
> > > > >>
> > > > >
> > > > > Do you mean re-mount while forgetting all changes to previous "volatile"
> > > > > mount?
> > > >
> > > > no, without forgetting them.
> > > > The original idea was to have a way to disable any sync operation in the
> > > > overlay file system and let the upper layers handle it.  IOW, mount
> > > > volatile overlay+umount overlay+syncfs upper dir must still be
> > > > considered safe.
> > > > If we want to make it safer and disallow remounting the same
> > > > workdir+upperdir by default when "volatile" is used, that is fine; but I
> > > > think there should still be a way to say "I know what I am doing, just
> > > > remount it".
> > >
> > > Indeed.  "Volatile" doesn't mean you can't use the data, just that the
> > > data may be lost completely in case of a crash (tmpfs analogue).
> > >
> > > Maybe just stick
> > >
> > >   $(workdir)/work/incompat/volatile/donotremove
> > >
> > > in there to prevent misuse.
> >
> > So we ask users to remove "$(workdir)/work/incompat/volatile/donotremove"
> > if they want to remount with with same upper/ and work/? (Presumably
> > after syncing upper/).
> >
> 
> Sounds right.
> Just don't rely on the workdir cleanup error yes?
> That protection is for old kernels and it falls back to r/o mount.
> New kernel should of course recognise $(workdir)/work/incompat/volatile
> fail to mount and explicitly error about unclean "volatile" unmount and
> maybe give a hint how to fix it in kernel log.

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.

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?

Thanks
Vivek

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

[Amir Goldstein]

> 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?

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?

Doesn't matter, either way, the same protection will be used.

Thanks,
Amir.

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

[Vivek Goyal]

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.

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.

Vivek

> 
> Doesn't matter, either way, the same protection will be used.
> 
> Thanks,
> Amir.
> 

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

[Amir Goldstein]

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.

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

[Miklos Szeredi]

On Tue, Aug 25, 2020 at 7:31 AM Amir Goldstein <amir73il@gmail.com> wrote:
>

>
> 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.

Yes, $workdir/work/incompat/volatile/dirty is good.

If exists, fail to mount overlay on new kernel (regardless of
"volatile" option); warn + r/o mount on old kernel.

In case mounting as "volatile", create it.

In no case does the kernel need to remove it.

Thanks,
Miklos

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

[Daniel Walsh]

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.