On 12 Apr 2015, at 21:02, Martin Pieuchot <[email protected]> wrote: It's only PoF just for to show my hypothetical roadmap. > Can you explain what need to be protected from what? > >> 1. Filelist, nfiles and operations with them are protected by rwlock. > > Why not, could you describe why they need a lock? A diff just to do > that would be much easier review. filelist is accessed by falloc(), fdrop(), unp_gc and sysctl_file() functions. After corresponding callers will be unlocked, their calls and filelist modifications/reading become asynchronous, so filelist need to be protected.
I want to have "read value -> or/and -> store value" chain not be broken by races, so f_iflags and f_flag operations are atomic. > >> 2. Garbage collector's flags FIF_MARK and FIF_DEFER moved from f_iflags to >> new field f_gc_flags (compatibility with pstat was kept). > > Why did you decide to split f_iflags in two? What's the problem this > move is solving? Here too a diff just to do that would be much easier > to review. Those flags exists only for sockets. I suppose they can be moved out from struct file but later. Those flags are modified/accessed in only one mp locked place, so they does'n need any protection now, and their modification doesn't affect on FIF_LARVAL and FIF_HASLOCK flags. >> 3. Operations over f_iflags are atomic. FIF_LARVAL set only once and >> FIF_HASLOCK isn't set too frequent. > > Why are you using atomic operations for these flags? Which scenario > that would help? Did you consider any alternative? FIF_LARVAL looks unnecessary, because file can become mature after f_ops field was set. Of course it is just assignment and it can be non-atomic. But now, there are some places, there f_ops methods called before FIF_LARVAL flag is set, so if FIF_LARVAL will gone, they should be rewritten before. FIF_HASLOCK can be removed too. it checked only in vn_closefile(), but this flag doesn’t indicate actual vnode lock state, because VOP_ADVLOCK()’s return value is not checked. May be it can be replaced by new vn_islocked() function, which will check actual vnode lock possibility and lock state? > >> 4. Set/unset operations over f_flag are atomic. They are not too frequent. > > Does that mean that as soon as the atomic operation has completed all the > conditions that a flag represent are true/false? Since the integrity of > the code you're changing is currently protected by the biglock the order > of the operations inside the functions does not really matter. Setting > the flag atomically is in generally not enough and since you're not > giving more details it's hard to dig into a huge diff ;) I want only f_flag consistence. mp locked related code must be reviewed later. >> 5. Operations over f_count are atomic. Surely those are frequent though they >> can be non atomic on uniprocessor kernel. > > Why do they need to be atomic and also why are you doing a dance with > the file_lock when you cannot increment the counter? fd_getfile_ref() has not any lock inside, so we have races with callers, which modify f_count. I want to have new value, incremented by 1 with previously known non-null value to avoid funref()/fd_gerfile_ref() races. If I can't increment counter, then it has been modified by someone else, so I re-check it's presence in fd_ofiles array, it's reference counter != 0 and if this fp was acqured, I check it's place in fd_ofiles again; fp != fp_ofiles[fd] means fp_ofiles[fd] was modified, fp actually was closed, and I should release it and try to acquire new file with given fd. >> 6. f_offset field is not protectd now, it should be protected later. > > From what should it be protected? Async dofilereadv(), dofilewritev() and sys_lseek() calls will modify this field. Also, off_t is 64bit and it should be protected for reading on 32bit architectures. > >> 7. Counters are not protected now, they should be protected later. > > Same question :) Same answer :) they are 64 bits and modify/read by async calls. > > Since I've been lurking in that area too recently, you'll find 5 > refactoring diffs attached to this email. It's easier for me to > send you the work that I did rather than comment on some things. I understand and agree with 0002, 0003, 0004 and 0005. I don't understand, why we need to check flags inside fd_getfile. > One more question, did you consider the fact that the code you're > modifying might contain some bugs which are currently not exposed > because most of it is run under the KERNEL_LOCK? Yes, of course. It can be painful process :)
