[CRIU] [PATCH] mount: handle mnt_flags and sb_flags separatly (v4)

[Tycho Andersen]

Hi Andrey,

I'm seeing a regression with this, something like:

(00.498962)      1: mnt:        Mounting tmpfs @./sys/fs/cgroup (0)
tar: ./cgmanager: Cannot mkdir: Read-only file system
tar: ./blkio: Cannot mkdir: Read-only file system
tar: ./cpu,cpuacct: Cannot mkdir: Read-only file system
tar: ./cpuset: Cannot mkdir: Read-only file system
tar: ./devices: Cannot mkdir: Read-only file system
tar: ./freezer: Cannot mkdir: Read-only file system
tar: ./hugetlb: Cannot mkdir: Read-only file system
tar: ./memory: Cannot mkdir: Read-only file system
tar: ./systemd: Cannot mkdir: Read-only file system
tar: ./net_cls,net_prio: Cannot mkdir: Read-only file system
tar: ./perf_event: Cannot mkdir: Read-only file system
tar: .: Cannot utime: Read-only file system
tar: .: Cannot change ownership to uid 0, gid 0: Read-only file system
tar: Exiting with failure status due to previous errors
(00.503409)      1: Error (util.c:593): exited, status=2
(00.503439)      1: Error (cr-restore.c:1234): 12 exited, status=2
(00.503450)      1: Error (mount.c:1254): mnt: Can't restore tmpfs content
(00.521198) Error (cr-restore.c:1933): Restoring FAILED.

(I wonder if there is some way we can avoid checkpointing this tmpfs since all
that's mounted there is cgroups, but that's a different issue.)

On Fri, Sep 11, 2015 at 04:55:55PM +0300, Andrey Vagin wrote:
> +/*
> + * Here are a set of flags which we know how to handle for the one mount call.
> + * All of them except MS_RDONLY are set only as mnt flags.
> + * MS_RDONLY is set for both mnt ans sb flags, so we can restore it for one
> + * mount call only if it set for both masks.
> + */
> +#define MS_MNT_KNOWN_FLAGS (MS_NOSUID | MS_NOEXEC | MS_NODEV | MS_NOATIME | \
> +				MS_NODIRATIME | MS_RELATIME | MS_RDONLY)
>  static int do_new_mount(struct mount_info *mi)
>  {
> -	unsigned long mflags = MS_SHARED | MS_PRIVATE | MS_SLAVE | MS_UNBINDABLE;
> +	unsigned long sflags = mi->sb_flags;
> +	unsigned long mflags = mi->flags & (~MS_PROPAGATE);
>  	char *src;
>  	struct fstype *tp = mi->fstype;
> -	bool remount_ro = (tp->restore && mi->flags & MS_RDONLY);
> +	bool remount_ro = (tp->restore && mi->sb_flags & MS_RDONLY);
>  
>  	src = resolve_source(mi);
>  	if (!src)
>  		return -1;
>  
>  	if (remount_ro)
> -		mflags |= MS_RDONLY;
> +		sflags &= ~MS_RDONLY;
> +
> +	/* Merge superblock and mount flags if it's posiable */
> +	if (!(mflags & ~MS_MNT_KNOWN_FLAGS) && ((sflags ^ mflags) & MS_RDONLY)) {
> +		sflags |= mflags;
> +		mflags = 0;
> +	}
>  
> -	if (mount(src, mi->mountpoint, tp->name,
> -			mi->flags & ~mflags, mi->options) < 0) {
> +	if (mount(src, mi->mountpoint, tp->name, sflags, mi->options) < 0) {

If sflags has MS_RDONLY here, doesn't that prevent us from doing the restore?

Tycho

>  		pr_perror("Can't mount at %s", mi->mountpoint);
>  		return -1;
>  	}
>  
> +	if (tp->restore && tp->restore(mi))
> +		return -1;
> +
> +	if (remount_ro)
> +		return mount(NULL, mi->mountpoint, tp->name,
> +			     MS_REMOUNT | MS_RDONLY, NULL);
> +
> +	if (mflags && mount(NULL, mi->mountpoint, NULL,
> +				MS_REMOUNT | MS_BIND | mflags, NULL)) {
> +		pr_perror("Unable to apply bind-mount options\n");
> +		return -1;
> +	}
> +
>  	if (restore_shared_options(mi, !mi->shared_id && !mi->master_id,
>  					mi->shared_id,
>  					mi->master_id))
> @@ -1960,12 +1990,6 @@ static int do_new_mount(struct mount_info *mi)
>  
>  	mi->mounted = true;
>  
> -	if (tp->restore && tp->restore(mi))
> -		return -1;
> -
> -	if (remount_ro)
> -		return mount(NULL, mi->mountpoint, tp->name,
> -			     MS_REMOUNT | MS_RDONLY, NULL);
>  	return 0;
>  }
>  
> @@ -1988,6 +2012,7 @@ static int do_bind_mount(struct mount_info *mi)
>  
>  	if (!mi->need_plugin) {
>  		char *root, *cut_root, rpath[PATH_MAX];
> +		unsigned long mflags;
>  
>  		if (mi->external) {
>  			/*
> @@ -2041,6 +2066,13 @@ do_bind:
>  			return -1;
>  		}
>  
> +		mflags = mi->flags & (~MS_PROPAGATE);
> +		if (!mi->bind || mflags != (mi->bind->flags & (~MS_PROPAGATE)))
> +			if (mount(NULL, mi->mountpoint, NULL, MS_BIND | MS_REMOUNT | mflags, NULL)) {
> +				pr_perror("Can't mount at %s", mi->mountpoint);
> +				return -1;
> +			}
> +
>  		if (unlikely(mi->deleted)) {
>  			if (S_ISDIR(st.st_mode)) {
>  				if (rmdir(root)) {
> @@ -2364,6 +2396,7 @@ static int collect_mnt_from_image(struct mount_info **pms, struct ns_id *nsid)
>  		pm->parent_mnt_id	= me->parent_mnt_id;
>  		pm->s_dev		= me->root_dev;
>  		pm->flags		= me->flags;
> +		pm->sb_flags		= me->sb_flags;
>  		pm->shared_id		= me->shared_id;
>  		pm->master_id		= me->master_id;
>  		pm->need_plugin		= me->with_plugin;
> diff --git a/proc_parse.c b/proc_parse.c
> index 3feaa44..82ce102 100644
> --- a/proc_parse.c
> +++ b/proc_parse.c
> @@ -1089,7 +1089,7 @@ static int parse_mountinfo_ent(char *str, struct mount_info *new, char **fsname)
>  	if (!new->options)
>  		goto err;
>  
> -	if (parse_sb_opt(opt, &new->flags, new->options))
> +	if (parse_sb_opt(opt, &new->sb_flags, new->options))
>  		goto err;
>  
>  	ret = 0;
> diff --git a/protobuf/mnt.proto b/protobuf/mnt.proto
> index f79ae46..12582e9 100644
> --- a/protobuf/mnt.proto
> +++ b/protobuf/mnt.proto
> @@ -43,4 +43,5 @@ message mnt_entry {
>  	optional bool		internal_sharing	= 15;
>  
>  	optional bool		deleted			= 16;
> +	optional uint32		sb_flags		= 17 [(criu).hex = true];
>  }
> -- 
> 2.4.3
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu