[CRIU] [PATCH v3 08/16] rst_info: artificial files list introduced
Pavel Emelyanov
xemul at parallels.com
Mon Dec 14 05:25:34 PST 2015
On 12/14/2015 03:59 PM, Stanislav Kinsburskiy wrote:
>
>
> 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.
IRL pipes are related to AutoFS and you use this fact anyway. Moreover, in
your patch you mess with generic fds layer from inside the autofs code -- the
find_fle_by_fd() and the way you call it is all about it.
> 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.
We've just seen how this made things hard to understand :( I agree that it's
handy to have the ability to call something after the fdt is populated and
have this ability w/o messing hard with the pipes.c code.
Artificial files solve this by obfuscating the files opening engine. If
having a way to call something after fdt is populated, then make it be
literally "something that is called after fdt is populated", i.e. a list
of dedicated objects that are queued up to rst_info and called _once_ at
the end of prepare_fds().
On the other hand what I'm proposing above seems to be a single call to the
collect_one_pipe() from autofs code.
> 3) You proposal means, that I have to drop almost all the code I wrote
> it, and implement it, debug it, etc from scratch.
Yes, sometimes good code to perfect code change is via complete rewrite.
-- Pavel
More information about the CRIU
mailing list