[CRIU] [PATCH v3 04/16] autofs: dump stage introduced

[Stanislav Kinsburskiy]

14.12.2015 12:56, Pavel Emelyanov пишет:
> On 12/14/2015 02:52 PM, Stanislav Kinsburskiy wrote:
>>
>> 14.12.2015 12:45, Pavel Emelyanov пишет:
>>> On 12/14/2015 02:12 PM, Stanislav Kinsburskiy wrote:
>>>
>>>>>> +
>>>>>> +	return ret;
>>>>>> +}
>>>>>> +
>>>>>> +static int autofs_fixup_options(struct mount_info *pm, int pgrp, bool catatonic,
>>>>>> +				int kernel_fd, bool kernel_fd_alive,
>>>>>> +				int read_fd)
>>>>>> +{
>>>>>> +	char **options;
>>>>>> +	int nr_opts, i, err = -1;
>>>>>> +	char *new_opt;
>>>>>> +	struct mount_info *t;
>>>>>> +
>>>>>> +	new_opt = construct_string("pgrp=%d,fd=%d", pgrp, kernel_fd);
>>>>>> +	if (!new_opt)
>>>>>> +		return -1;
>>>>>> +
>>>>>> +	if (!catatonic && !kernel_fd_alive) {
>>>>>> +		/* Write end is closed or invalid
>>>>>> +		 * Let's introduce new options only for us to carry pipe
>>>>>> +		 * information. */
>>>>> So, do I get it right, that the only reason for doing the complex write-end
>>>>> discovery is to decide whether or not to keep read_fd=%d in the image?
>>>> I don't understand. Write end is known and just checked for presence and
>>>> validity.
>>>> Discovery is done for read end. And we do it for two purposes:
>>>> 1) Check, that it's empty
>>>> 2) Dump, if write end is closed.
>>> I'm talking about the kernel_fd_alive variable. It's set to true after calling
>>> the autofs_kernel_pipe_alive() and is only used here, to check whether or not
>>> to keep the read_fd=%d option in the image or not. Also, the only thing for
>>> which the autofs_kernel_pipe_alive() is used is to set the kernel_fd_alive bool.
>>>
>>> So my question is -- why not keep the read_fd=%d option always and remove the
>>> whole autofs_kernel_pipe_alive() routine?
>> Because "read_fd" option is used on restore stage to device whether or
>> not temporary write end must be created.
>> In case of this option is absent, only artificial file is created to be
>> used to fix up mount point.
>> Otherwise there must be added write end of the pipe to file descriptors,
>> which must be closed after mount is fixed.
> Hm... OK, let's consider two autofs mount points and master tasks.
> The first -- with only read pipe end opened at task and write end opened at
> mountpoint (superblock). The second mountpoint has write end at superblock
> and task has both -- read and write ends. What would be the difference in
> option set and restore procedure?

For the first one:
1) Create temporary write pipe end and add it to process fds, pipe_info, 
etc.
2) Create artificial fd for pipe write end, which will be used to fixup 
the mount (and also close temporary write_fd).

For the second one:
1) Create artificial fd for pipe write end, which will be used to fixup 
the mount.


>>>>>> +		new_opt = add_to_string(new_opt, ",read_fd=%d", read_fd);
>>>>> If you need to keep some fs-specific data in the image that is not going to
>>>>> be passed as-is into the mount() call, just introduce the necessary optional
>>>>> fields in the mnt proto file.
>>>> This mnt proto file is generic.
>>>> Do you want me to add some autofs-specific fild to it (which can be
>>>> unset, btw, thus I need another boolean flag)?
>>> Well, this can be done in two ways. The first is to add optional field into the
>>> mnt entry. The second is to introduce one more image file (like for tmpfs).
>>>
>>> Keeping this in options string is also an option, but if this particular
>>> option is never passed into mount() system call and you parse this string
>>> anyway, I propose to save the cpu cycles and keep the desired data out of
>>> this string.
>> It will make code more complex, and, frankly, I don't see any
>> significant reason for this.
> Right now you keep read_fd value as sub-string inside the options one and parse
> one from there on restore. In my proposal the read_fd value will sit as integer
> right in the mnt_entry. Will it?

Sure, it will. But mnt_entry is generic.
I suppose it's nice to put there some fs-private information in explicit 
way, don't you think?

>> But I can do it, if you insist.
>>
>>>>>> +	}
>>>>>> +	if (!new_opt)
>>>>>> +		return -1;
>>>>>> +
>>>>>> +	split(pm->options, ',', &options, &nr_opts);
>>>>>> +	if (!options)
>>>>>> +		return -1;
>>>>>> +
>>>>>> +	for (i = 0; i < nr_opts; i++) {
>>>>>> +		char *opt = options[i];
>>>>>> +
>>>>>> +		if (!strncmp(opt, "pgrp=", strlen("pgrp=")) ||
>>>>>> +		    !strncmp(opt, "fd=", strlen("fd=")) ||
>>>>>> +		    !strncmp(opt, "pipe_ino=", strlen("pipe_ino=")))
>>>>>> +			continue;
>>>>>> +
>>>>>> +		new_opt = add_to_string(new_opt, ",%s", opt);
>>>>>> +		if (!new_opt)
>>>>>> +			goto out;
>>>>>> +	}
>>>>>> +
>>>>>> +	xfree(pm->options);
>>>>>> +	pm->options = new_opt;
>>>>>> +
>>>>>> +	/* Change options for all bind-mounts */
>>>>> What for?
>>>> To make sure, that mount will be called with the options I fixed, and
>>>> not with original ones (like it can be, because actual mount point to
>>>> create is selected randomly).
>>> But the bind mounts will be called like this
>>>
>>>         mount(foo, bar, NULL, MS_BIND, NULL);
>>>
>>> without any knowledge about the options.
>>>
>> Yes. BUt we don't know, which mount point will be selected ad the one to
>> restore.
>> Frankly, it's possibly determined somehow. But in my experiments mount
>> with fixed options was always one of the bind-mount and not the the one
>> to create.
> OK. Write this in the comment to this place in the code.

Sure.

> -- Pavel
>