[CRIU] [PATCH v3 08/16] rst_info: artificial files list introduced

[Stanislav Kinsburskiy]

14.12.2015 13:42, Pavel Emelyanov пишет:
> On 12/14/2015 03:18 PM, Stanislav Kinsburskiy wrote:
>>
>> 14.12.2015 13:00, Pavel Emelyanov пишет:
>>> On 12/14/2015 02:55 PM, Stanislav Kinsburskiy wrote:
>>>> 14.12.2015 12:47, Pavel Emelyanov пишет:
>>>>> On 12/14/2015 02:16 PM, Stanislav Kinsburskiy wrote:
>>>>>> 14.12.2015 12:10, Pavel Emelyanov пишет:
>>>>>>> On 12/10/2015 06:16 PM, Stanislav Kinsburskiy wrote:
>>>>>>>> This list is used to collect and handle some service files, which an be used
>>>>>>>> only during restore procedure for some service perations, and must be closed
>>>>>>>> before process is released.
>>>>>>> OK, but I see only opening code in here, where's the closing one?
>>>>>> I do close in post_open() callback.
>>>>>> If there is a better place, I would be glad to do it there.
>>>>> In the post_open_fd() right after calling the ->post_open?
>>>>>
>>>> Emm. How to distinguish, which fd have to closed and which has not?
>>>> By introducing another optional callback?
>>> You already know which list you're scanning -- artificial or not -- so it's
>>> possible to make this decision based on this knowledge.
>>>
>>> Otherwise I see no benefits of having the 4th list on rst_info over the
>>> specially crafted desc_opts (as you do now anyway) that would just close the
>>> descriptor itself in its post_open callback.
>> You, probably, thinking about something, which you are not exposing to me.
>> Function post_open_fd() doesn't have information about the list.
> It doesn't have list as an argument, yes, but simply because the list is
> dropped from arg list one call before it.
>
>> But it has information about desc. I can check it for typeof ARTIFICIAL,
>> but you tend to remove this type, don't you?
> No I don't I ask you (if we keep the artificial files concept at all) just
> to move this constant out of protobuf files.

Ok.

>>>>>>>> AutoFS will use them to fix up mount points druing restore.
>>>>>>> Why can't we use existing fds list?
>>>>>> We can, if we put them at the end on the list.
>>>>> Why at the end of the list?
>>>> Because artificial files is just a way to operate on existent ones. Thus
>>>> existent ones have to be processes (opened) befoe artificial ones.
>>> No, it's not so. In the current set artificial files are the same ones (pipes)
>>> that are anyway owned be the processes we restore. So you can pretend that
>>> the process that _wants_ autofs pipe for mountpoint final fixup just shares
>>> the pipe with the process that actually _owns_ one in its fdt.
>> I'm sorry, If I understand you wrong, but _artificial_ files are "owned"
>> by a process only in terms that they are traversed within process
>> address space.
>> There are no real opened files descriptors for them. They are pure virtual.
> You close() one at the end of the post_open callback.

Oops... That's a bug. Close fails here obviously.
This piece of code have to be removed.

>> The main intention to introduce them was to have some object, which can
>> be used to call a callback in the proper time with proper _real_ files
>> already opened.
>> By having this object, all AutoFS hacks can be isolated within Autofs
>> code, instead of introducing some callbacks or checks in generic pipes code.
>> And pipe write end itself is _not_ aritificial. It's real fd in fds
>> list. But it have to be closed after fixup.
>> Fixup and close of write end are done in post_open() callback of
>> _artificial_ fd.
> And artificial fd is created at the task which has the real pipe in his fdt
> to set this real fd into the superblock. I had an impression that the artificial
> fd is just another descriptor for the real write pipe. OK, I was wrong reading
> the patchset.
>
> Why can't we make the write-end setting to look like I described above? In
> details:
>
> 1. Find the file object that corresponds to the write-end of the pipe that should
>     sit in the kernel.
>
> 1a. If the write end is not there -- add one and use it.
>
> Note, that I don't propose to create artificial files, I propose to use existing
> pipe stuff, i.e. -- the pipe_info object.
>
> 2. Find the file descriptor that corresponds to the pipe object from step 1.
>
> 2a. If the file descriptor is absent _or_ sits in the wrong task -- add fake
>      file descriptor
>
> 3. In pipe_post_open (add such) fixup the mountpoint and close() the descriptor
>     if required.

Yes, it can be done this way. The benefit I see here is avoiding of 
introducing an object, which we can live without. Like artificial files. 
Occam razor is nice,
but:
1) It means, that AutoFS code and pipe code are interconnected. I 
believe it's something, which should be avoided. Especially because 
usually, in real life, pipes won't be related to AutoFS.
2) I believe, that for debugging purposes, it's better to have 
independent pieces of code be isolated as much, as possible. In case of 
AutoFS - especially.
Current implementation can be modified, while "spaghetti" code approach 
(like kernel CPT in OpenVZ) makes things harder to understand, develop 
and debug.
IOW, in this particular case I believe, that introducing one more (quite 
generic, btw) object like "artificial file" worth it.
3) You proposal means, that I have to drop almost all the code I wrote 
it, and implement it, debug it, etc from scratch.



>> Did I miss something from you question?
> -- Pavel