[v2,3/5] mount: save ext_real_root for external mounts

Submitted by Pavel Tikhomirov on March 20, 2017, 9:45 a.m.

Details

Message ID 20170320094544.15391-4-ptikhomirov@virtuozzo.com
State New
Series "mount: handle external mount bindmounts"
Headers show

Commit Message

Pavel Tikhomirov March 20, 2017, 9:45 a.m.
need it to check if we can bindmount from external mount

https://jira.sw.ru/browse/PSBM-46753

v2: s/real_root/ext_real_root/
Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
---
 criu/mount.c     | 3 ++-
 images/mnt.proto | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/criu/mount.c b/criu/mount.c
index 6e5a4c6..ef5c1ec 100644
--- a/criu/mount.c
+++ b/criu/mount.c
@@ -1335,6 +1335,7 @@  static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
 		 * for reverse mapping details.
 		 */
 		me.root	= pm->external;
+		me.ext_real_root = pm->root;
 		me.has_ext_mount = true;
 		me.ext_mount = true;
 	} else
@@ -2445,7 +2446,7 @@  static int get_mp_root(MntEntry *me, struct mount_info *mi)
 {
 	char *ext = NULL;
 
-	mi->root = xstrdup(me->root);
+	mi->root = xstrdup(me->ext_real_root ? : me->root);
 	if (!mi->root)
 		return -1;
 
diff --git a/images/mnt.proto b/images/mnt.proto
index 50cd8af..3294319 100644
--- a/images/mnt.proto
+++ b/images/mnt.proto
@@ -53,4 +53,5 @@  message mnt_entry {
 
 	optional bool		deleted			= 16;
 	optional uint32		sb_flags		= 17 [(criu).hex = true];
+	optional string		ext_real_root		= 18;
 }

Comments

Andrey Vagin March 20, 2017, 7:03 p.m.
On Mon, Mar 20, 2017 at 12:45:42PM +0300, Pavel Tikhomirov wrote:
> need it to check if we can bindmount from external mount
> 
> https://jira.sw.ru/browse/PSBM-46753
> 
> v2: s/real_root/ext_real_root/
> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> ---
>  criu/mount.c     | 3 ++-
>  images/mnt.proto | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/criu/mount.c b/criu/mount.c
> index 6e5a4c6..ef5c1ec 100644
> --- a/criu/mount.c
> +++ b/criu/mount.c
> @@ -1335,6 +1335,7 @@ static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
>  		 * for reverse mapping details.
>  		 */
>  		me.root	= pm->external;
> +		me.ext_real_root = pm->root;
>  		me.has_ext_mount = true;
>  		me.ext_mount = true;
>  	} else
> @@ -2445,7 +2446,7 @@ static int get_mp_root(MntEntry *me, struct mount_info *mi)
>  {
>  	char *ext = NULL;
>  
> -	mi->root = xstrdup(me->root);
> +	mi->root = xstrdup(me->ext_real_root ? : me->root);
>  	if (!mi->root)
>  		return -1;
>  
> diff --git a/images/mnt.proto b/images/mnt.proto
> index 50cd8af..3294319 100644
> --- a/images/mnt.proto
> +++ b/images/mnt.proto
> @@ -53,4 +53,5 @@ message mnt_entry {
>  
>  	optional bool		deleted			= 16;
>  	optional uint32		sb_flags		= 17 [(criu).hex = true];
> +	optional string		ext_real_root		= 18;

We need a comment here what this means. What do you think if we add
ext_key instead of ext_real_root, for me it looks more understandable.
>  }
> -- 
> 2.9.3
>
Pavel Tikhomirov March 21, 2017, 7:53 a.m.
On 03/20/2017 10:03 PM, Andrew Vagin wrote:
> On Mon, Mar 20, 2017 at 12:45:42PM +0300, Pavel Tikhomirov wrote:
>> need it to check if we can bindmount from external mount
>>
>> https://jira.sw.ru/browse/PSBM-46753
>>
>> v2: s/real_root/ext_real_root/
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ---
>>  criu/mount.c     | 3 ++-
>>  images/mnt.proto | 1 +
>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/criu/mount.c b/criu/mount.c
>> index 6e5a4c6..ef5c1ec 100644
>> --- a/criu/mount.c
>> +++ b/criu/mount.c
>> @@ -1335,6 +1335,7 @@ static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
>>  		 * for reverse mapping details.
>>  		 */
>>  		me.root	= pm->external;
>> +		me.ext_real_root = pm->root;
>>  		me.has_ext_mount = true;
>>  		me.ext_mount = true;
>>  	} else
>> @@ -2445,7 +2446,7 @@ static int get_mp_root(MntEntry *me, struct mount_info *mi)
>>  {
>>  	char *ext = NULL;
>>
>> -	mi->root = xstrdup(me->root);
>> +	mi->root = xstrdup(me->ext_real_root ? : me->root);
>>  	if (!mi->root)
>>  		return -1;
>>
>> diff --git a/images/mnt.proto b/images/mnt.proto
>> index 50cd8af..3294319 100644
>> --- a/images/mnt.proto
>> +++ b/images/mnt.proto
>> @@ -53,4 +53,5 @@ message mnt_entry {
>>
>>  	optional bool		deleted			= 16;
>>  	optional uint32		sb_flags		= 17 [(criu).hex = true];
>> +	optional string		ext_real_root		= 18;
>
> We need a comment here what this means. What do you think if we add
> ext_key instead of ext_real_root, for me it looks more understandable.

If we use me->root for real root, and me->ext_key for the key of the 
mount, that would surely break backward compatibility, as old criu will 
still want the key in me->root.

Thus we should leave the key of the mount in me->root. And naming the 
field in which I want to save the real root(path in the file system to 
the root of these mount) as me->ext_key is counterintuitive to me.

>>  }
>> --
>> 2.9.3
>>
Andrey Vagin March 22, 2017, 1:02 a.m.
On Tue, Mar 21, 2017 at 10:53:25AM +0300, Pavel Tikhomirov wrote:
> 
> 
> On 03/20/2017 10:03 PM, Andrew Vagin wrote:
> > On Mon, Mar 20, 2017 at 12:45:42PM +0300, Pavel Tikhomirov wrote:
> > > need it to check if we can bindmount from external mount
> > > 
> > > https://jira.sw.ru/browse/PSBM-46753
> > > 
> > > v2: s/real_root/ext_real_root/
> > > Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
> > > ---
> > >  criu/mount.c     | 3 ++-
> > >  images/mnt.proto | 1 +
> > >  2 files changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/criu/mount.c b/criu/mount.c
> > > index 6e5a4c6..ef5c1ec 100644
> > > --- a/criu/mount.c
> > > +++ b/criu/mount.c
> > > @@ -1335,6 +1335,7 @@ static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
> > >  		 * for reverse mapping details.
> > >  		 */
> > >  		me.root	= pm->external;
> > > +		me.ext_real_root = pm->root;
> > >  		me.has_ext_mount = true;
> > >  		me.ext_mount = true;
> > >  	} else
> > > @@ -2445,7 +2446,7 @@ static int get_mp_root(MntEntry *me, struct mount_info *mi)
> > >  {
> > >  	char *ext = NULL;
> > > 
> > > -	mi->root = xstrdup(me->root);
> > > +	mi->root = xstrdup(me->ext_real_root ? : me->root);
> > >  	if (!mi->root)
> > >  		return -1;
> > > 
> > > diff --git a/images/mnt.proto b/images/mnt.proto
> > > index 50cd8af..3294319 100644
> > > --- a/images/mnt.proto
> > > +++ b/images/mnt.proto
> > > @@ -53,4 +53,5 @@ message mnt_entry {
> > > 
> > >  	optional bool		deleted			= 16;
> > >  	optional uint32		sb_flags		= 17 [(criu).hex = true];
> > > +	optional string		ext_real_root		= 18;
> > 
> > We need a comment here what this means. What do you think if we add
> > ext_key instead of ext_real_root, for me it looks more understandable.
> 
> If we use me->root for real root, and me->ext_key for the key of the mount,
> that would surely break backward compatibility, as old criu will still want
> the key in me->root.

If .ext_mount is true and .has_ext_key is true, you need to get key from
.ext_key, otherwise we get it from .root. Maybe you mean that we will
not be able to restore processes with old criu, if they have been dumped
them by a new one. It is true, but we don't require this type of
compatibility. I think it's better to have a clean images.

> 
> Thus we should leave the key of the mount in me->root. And naming the field
> in which I want to save the real root(path in the file system to the root of
> these mount) as me->ext_key is counterintuitive to me.
> 
> > >  }
> > > --
> > > 2.9.3
> > > 
> 
> -- 
> Best regards, Tikhomirov Pavel
> Software Developer, Virtuozzo.
Pavel Emelianov March 23, 2017, 8:36 a.m.
On 03/21/2017 10:53 AM, Pavel Tikhomirov wrote:
> 
> 
> On 03/20/2017 10:03 PM, Andrew Vagin wrote:
>> On Mon, Mar 20, 2017 at 12:45:42PM +0300, Pavel Tikhomirov wrote:
>>> need it to check if we can bindmount from external mount
>>>
>>> https://jira.sw.ru/browse/PSBM-46753
>>>
>>> v2: s/real_root/ext_real_root/
>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>> ---
>>>  criu/mount.c     | 3 ++-
>>>  images/mnt.proto | 1 +
>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/criu/mount.c b/criu/mount.c
>>> index 6e5a4c6..ef5c1ec 100644
>>> --- a/criu/mount.c
>>> +++ b/criu/mount.c
>>> @@ -1335,6 +1335,7 @@ static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
>>>  		 * for reverse mapping details.
>>>  		 */
>>>  		me.root	= pm->external;
>>> +		me.ext_real_root = pm->root;
>>>  		me.has_ext_mount = true;
>>>  		me.ext_mount = true;
>>>  	} else
>>> @@ -2445,7 +2446,7 @@ static int get_mp_root(MntEntry *me, struct mount_info *mi)
>>>  {
>>>  	char *ext = NULL;
>>>
>>> -	mi->root = xstrdup(me->root);
>>> +	mi->root = xstrdup(me->ext_real_root ? : me->root);
>>>  	if (!mi->root)
>>>  		return -1;
>>>
>>> diff --git a/images/mnt.proto b/images/mnt.proto
>>> index 50cd8af..3294319 100644
>>> --- a/images/mnt.proto
>>> +++ b/images/mnt.proto
>>> @@ -53,4 +53,5 @@ message mnt_entry {
>>>
>>>  	optional bool		deleted			= 16;
>>>  	optional uint32		sb_flags		= 17 [(criu).hex = true];
>>> +	optional string		ext_real_root		= 18;
>>
>> We need a comment here what this means. What do you think if we add
>> ext_key instead of ext_real_root, for me it looks more understandable.
> 
> If we use me->root for real root, and me->ext_key for the key of the 
> mount, that would surely break backward compatibility, as old criu will 
> still want the key in me->root.

There's no need in letting older criu restore stuff from newer images.
If we think that the existing field should be used the other way, then
rename it to ${old_name}_deprecated in the image, introduce the new one
and always use it.

> Thus we should leave the key of the mount in me->root. And naming the 
> field in which I want to save the real root(path in the file system to 
> the root of these mount) as me->ext_key is counterintuitive to me.
> 
>>>  }
>>> --
>>> 2.9.3
>>>
>
Pavel Tikhomirov March 23, 2017, 8:50 a.m.
On 03/23/2017 11:36 AM, Pavel Emelyanov wrote:
> On 03/21/2017 10:53 AM, Pavel Tikhomirov wrote:
>>
>>
>> On 03/20/2017 10:03 PM, Andrew Vagin wrote:
>>> On Mon, Mar 20, 2017 at 12:45:42PM +0300, Pavel Tikhomirov wrote:
>>>> need it to check if we can bindmount from external mount
>>>>
>>>> https://jira.sw.ru/browse/PSBM-46753
>>>>
>>>> v2: s/real_root/ext_real_root/
>>>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>>>> ---
>>>>  criu/mount.c     | 3 ++-
>>>>  images/mnt.proto | 1 +
>>>>  2 files changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/criu/mount.c b/criu/mount.c
>>>> index 6e5a4c6..ef5c1ec 100644
>>>> --- a/criu/mount.c
>>>> +++ b/criu/mount.c
>>>> @@ -1335,6 +1335,7 @@ static int dump_one_mountpoint(struct mount_info *pm, struct cr_img *img)
>>>>  		 * for reverse mapping details.
>>>>  		 */
>>>>  		me.root	= pm->external;
>>>> +		me.ext_real_root = pm->root;
>>>>  		me.has_ext_mount = true;
>>>>  		me.ext_mount = true;
>>>>  	} else
>>>> @@ -2445,7 +2446,7 @@ static int get_mp_root(MntEntry *me, struct mount_info *mi)
>>>>  {
>>>>  	char *ext = NULL;
>>>>
>>>> -	mi->root = xstrdup(me->root);
>>>> +	mi->root = xstrdup(me->ext_real_root ? : me->root);
>>>>  	if (!mi->root)
>>>>  		return -1;
>>>>
>>>> diff --git a/images/mnt.proto b/images/mnt.proto
>>>> index 50cd8af..3294319 100644
>>>> --- a/images/mnt.proto
>>>> +++ b/images/mnt.proto
>>>> @@ -53,4 +53,5 @@ message mnt_entry {
>>>>
>>>>  	optional bool		deleted			= 16;
>>>>  	optional uint32		sb_flags		= 17 [(criu).hex = true];
>>>> +	optional string		ext_real_root		= 18;
>>>
>>> We need a comment here what this means. What do you think if we add
>>> ext_key instead of ext_real_root, for me it looks more understandable.
>>
>> If we use me->root for real root, and me->ext_key for the key of the
>> mount, that would surely break backward compatibility, as old criu will
>> still want the key in me->root.
>
> There's no need in letting older criu restore stuff from newer images.
> If we think that the existing field should be used the other way, then
> rename it to ${old_name}_deprecated in the image, introduce the new one
> and always use it.

Please, see "[PATCH v5 1/3] mount: save ext_real_root for external mounts"

I've resent the patchset, and in v5 I made mnt_entry->root always show 
root - no more mapping in it. And at the same time mnt_entry->ext_mount 
will always be false, so that old criu will just see no external mounts 
provided in new image. So no need to deprecate mnt_entry->root, we can 
reuse it.

Now external mount mapping is determined on me->ext_key field check, so 
I need ext_mount only for forward compatibility, we can deprecate it 
later if needed.

>
>> Thus we should leave the key of the mount in me->root. And naming the
>> field in which I want to save the real root(path in the file system to
>> the root of these mount) as me->ext_key is counterintuitive to me.
>>
>>>>  }
>>>> --
>>>> 2.9.3
>>>>
>>
>