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

[Stanislav Kinsburskiу]

14 дек. 2015 г. 14:25 пользователь Pavel Emelyanov <xemul at parallels.com> написал:
>
> 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. 
>

We can' avoid of calling pipes callbacks from autofs anyway.
Autofs is related to pipes, but not vise versa. Isn't it better to keep all the hacks in Autofs code, which can't survive without pipes, then in generic pipes code?

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

I don't think that they are hard to understand. Simple virtual objects to do some tricks without introducing more connections between weakly connected pieces of code. If you want to rename artificial fds into something else - this makes some sense.

> Artificial files solve this by obfuscating the files opening engine.

By obfuscating you mean introducing one more list to iterate while opening?


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

This can be done. But in case of AutoFs they will be file-related object and operations anyway.
If you think, that it's better to introduce another object, I can do it.

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

Nothing to object to such a statement. But I don't see any significant evidence for now, why it should be done and how you proposal leads to perfectness.
If we are talking about obfuscating, then pipes code is already obfuscated enough by introducing one more place, where fds are send via unix socket. It took some time for me to find this place and recall, why it was done (but I still don't really get, why it was implemented _this_ way, instead of keeping "generic fds" code, which you value a lot, simple and strict).
The thing you propose adds more obfuscating connections with other code pieces to pipe restore logic.
In spite of it allows to reduce amount of code lines, I believe it makes pipes.c even more harder to understand. And Autofs restore won't be clear from a brief look as well.

But, frankly, I don't want to argue, because it's pointless.
I can do it the way you want just to fiinish this task.

> -- Pavel