On Sun, Jun 24, 2018 at 12:13 AM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote: > On Sat, Jun 23, 2018 at 05:54:47PM +0300, Amir Goldstein wrote: >> The object marks manipulation functions fsnotify_destroy_marks() >> fsnotify_find_mark() and their helpers take an argument of type >> struct fsnotify_mark_connector __rcu ** to dereference the connector >> pointer. use a typedef to describe this type for brevity. > > That kind of typedefs is generally considered a bad style kernel-side... Yeh... look. Before we decide fold and open code struct fsnotify_mark_connector __rcu ** everywhere making fsnotify code uglier and harder to understand (IMO), I'll just say that I still hold the opinion that struct fsnotify_obj *is* the correct coding pattern here: https://marc.info/?l=linux-fsdevel&m=152426580218852&w=2 The rejections we got from Linus on v3 were just - 1. We were not able to guaranty that the change doesn't grow struct inode doesn't grow on some architectures/compilers 2. Linus dislikes seeing embedded structs in struct inode, that may silently grow in the future, below his radar 3. Linus generally preferred if we could get along with an abstract pointer in struct inode not having to include another header file in fs.h For the sake of considering all options, I would like to propose that we bring back fsnotify_obj, but let it contain only a connector pointer, leaving i_fsnotify_mask independent. Doing so will have addressed the main concern about struct inode size. Concern #2 can be addressed by documenting the size of fsnotify_obj in a comment in struct inode and adding BUILD_BUG_ON to preserve that guaranty. Concern #3 will need to remain and be a compromise for the sake of better (IMO) fsnotify code. Thought? Amir.
