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

[Pavel Emelyanov]

On 12/14/2015 03:07 PM, Stanislav Kinsburskiy wrote:
> 
> 
> 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.

OK, and in the 2nd case the artificial fd will be "attached" to some existing
pipe-object that is the one sitting the the tasks fds, right?

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

This is what I propose, but protobuf doesn't allow for void *field :) You would
have to introduce the 'optional autofs_private autofs' field in the mnt_entry itself.