On 27/11/17(Mon) 19:32, Mathieu - wrote: > Martin Pieuchot wrote: > > On 27/11/17(Mon) 11:49, Mathieu - wrote: > > > Hi everyone, > > > > > > I was looking / poking around the filedesc handling in kern_descrip.c > > > and found the locking a bit.. weird, > > > > Can you define "weird"? Is it a taste thing or did you find any bug? > > If it's a bug how can you reproduce it? > > Well it's a mix. Yeah a bit of a personnal taste (which means nothing > here), but also the fact that the current locking looks buggy to me (see > below).
Changes for personal taste should be avoided. > > > especially the fd_getfile function > > > is touching protected members of the filedesc w/o taking any lock. > > > > Which "protected members" are you talking about? What makes you think > > they are protected? What do you mean with protected? Why do you think > > they need a lock? > > I'm speaking of the fd_ofiles, fd_ofileflags and fd_(lo|hi)map members > of the filedesc struct. And by protected I mean that their access needs > to be done with the fd_lock held. (It's hinted in the struct commentaries, > and the lock was introduced to prevent racy access of those members). You're not giving any fact. You're argumentation is poor. What you're saying is "it should be like that". Well that might be true, but unless you can say "why" nobody is going to work with you. > > > This has been already hit previously in [1] and fixed by reordering the > > > malloc / free operations in rev 1.138 of kern_descrip.c ([2]) > > > > The bug that has been found has been fixed. What are you trying to do? > > Yes it's been fixed but what I argue, is that fd_getfile is still > touching fd_ofiles w/o serializing the access wrt to the writes. Where, how, what? Fact, fact, fact, fact! Stop the bullshit. Give concrete examples, stop wasting our time :) > So while the bug is fixed, that I agree with you, what I mean is that > fd_getfile is error prone in its current form and can be wrongly used. Still no fact, a lot of words. > > > At the time it was proposed to actually lock fd_getfile which entailed > > > fixing all the callers. > > > > This could be useful for a different reason. Can you guess which one? Why didn't you answer this question? > > > I tried to go this way but it proved difficult > > > as sometimes the locking is done one level higher (e.g namei() being > > > called with or without the filedesc lock held). > > > called with or without the filedesc lock held). Introducing a _locked > > > version of the file was also not feasible for the same reasons (and > > > anyway it was a bit ugly). > > > > Yes it's a complicated task. > > It is, and, imho it makes things a lot more complex when trying to solve > it this way. Some times complexity is needed. We cannot decide anything yet. We don't have the required information. > > > So I went down the rabbit hole and tried changing the semantics of the > > > whole filedesc manipulations in order to make this clearer. > > > > Can you define "this"? > > Sorry, english not being my first language this wasn't as clear as I > intended it to be. By "this" I meant to say, "the locking semantics" (of > filedesc). You're English is fine, your reasoning is not. It is too abstract. What do you mean by "the locking semantics"? > > > I know the diff is a bit big and scary because it touches sensitive > > > paths... but I wanted to have an opinion on it before going further. > > > > Before you go any further you need: > > > > - to explain what you want to achieve > > - to have a way to test your changes > > - to convince us that this is what we want > > > > Moving the lock above a level might be something we want. But every > > time you move the lock/unlock dance to a caller function you might > > introduce a deadlock. So a big diff is not the way to go. You might > > also not need to fix all the code paths at once. It depends what you're > > trying to achieve. > > > Well it all started with looking at file_desc handling for fun. My > endgame, as it seems that's what you are asking, is to make the filedesc > code mpsafe. That's a pretty big task, you are one to know!, so on the > road to there what I wanted first was to get the locking more > straightforward than it is (and that's why I mentionned FIF_LARVAL but > let's not go this way ..) I'm not sure what you mean with straightforward. What I understand from your diff is that it means "try to put everything under a lock without understanding". Understanding is more important than the diffs you're sending. Because then you can debug the problems. If you don't understand what needs to be protected from what in which situation you're doomed to revert your diff and try, try and try without being sure to succeed. > > > It has been running here for a few days, on a workstation, I also ran > > > the regress tests w/o any regressions. > > > > On which filesystems did you build the system? Which threaded > > application did you ran? > > ffs, nothing pretty special about it. I used heavily the regression > tests and the userland build as a stress test (it seems to me to be a > good test when it comes to file handling) ffs? Are you kidding? You think I'm asking question to be rude? I'm not. I wanted to know if you tried NFS. Because that's where all the bugs are lurking. System programming is hard because when doing such change you'll expose bugs that have very little to do with your original diff. > > I'd suggest you to split your changes in small reviewable diffs. Make > > sure you have a goal. Make sure you can test it. You might need to > > write tests for that. > > As I'm changing the way the locking is done I must admit splitting it > won't be easy, but it's an interesting task I'll need to think about it. That'd nice. > As for the tests, yes, I need more robust test. Yes because the regressions do not cover complex multi-threaded programs so you won't exercise parallelism on the code you're changing. > Thanks for all the feedback. You're welcome.
