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

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

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

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

-- Pavel