| Message ID | 1529765691-25170-2-git-send-email-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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...
On Sun, Jun 24, 2018 at 12:13 AM, Al Viro <viro@zeniv.linux.org.uk> 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.
On Sat 23-06-18 22:13:41, Al Viro 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... I know that such typedefs are discouraged as they often reduce readability (you have to look up the typedef to know what the type is about). However in this case the type name is long enough and we also often pass 'pointer to pointer' so that I think the readability of the result is actually better. And Amir has chosen reasonably clear name fsnotify_connp_t to show this is about a pointer to fsnotify_connector so that you should not need to look up the typedef more than once ;) Honza
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h index 34515d2c4ba3..94cedf8264ba 100644 --- a/fs/notify/fsnotify.h +++ b/fs/notify/fsnotify.h @@ -19,8 +19,8 @@ extern struct srcu_struct fsnotify_mark_srcu; extern int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b); -/* Destroy all marks connected via given connector */ -extern void fsnotify_destroy_marks(struct fsnotify_mark_connector __rcu **connp); +/* Destroy all marks attached to an object via connector */ +extern void fsnotify_destroy_marks(fsnotify_connp_t *connp); /* run the list of all marks associated with inode and destroy them */ static inline void fsnotify_clear_marks_by_inode(struct inode *inode) { diff --git a/fs/notify/mark.c b/fs/notify/mark.c index 61f4c5fa34c7..7b595acd8ec9 100644 --- a/fs/notify/mark.c +++ b/fs/notify/mark.c @@ -436,10 +436,9 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b) return -1; } -static int fsnotify_attach_connector_to_object( - struct fsnotify_mark_connector __rcu **connp, - struct inode *inode, - struct vfsmount *mnt) +static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp, + struct inode *inode, + struct vfsmount *mnt) { struct fsnotify_mark_connector *conn; @@ -476,7 +475,7 @@ static int fsnotify_attach_connector_to_object( * they are sure list cannot go away under them. */ static struct fsnotify_mark_connector *fsnotify_grab_connector( - struct fsnotify_mark_connector __rcu **connp) + fsnotify_connp_t *connp) { struct fsnotify_mark_connector *conn; int idx; @@ -508,7 +507,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark, { struct fsnotify_mark *lmark, *last = NULL; struct fsnotify_mark_connector *conn; - struct fsnotify_mark_connector __rcu **connp; + fsnotify_connp_t *connp; int cmp; int err = 0; @@ -629,9 +628,8 @@ int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode, * Given a list of marks, find the mark associated with given group. If found * take a reference to that mark and return it, else return NULL. */ -struct fsnotify_mark *fsnotify_find_mark( - struct fsnotify_mark_connector __rcu **connp, - struct fsnotify_group *group) +struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp, + struct fsnotify_group *group) { struct fsnotify_mark_connector *conn; struct fsnotify_mark *mark; @@ -697,8 +695,8 @@ void fsnotify_clear_marks_by_group(struct fsnotify_group *group, } } -/* Destroy all marks attached to inode / vfsmount */ -void fsnotify_destroy_marks(struct fsnotify_mark_connector __rcu **connp) +/* Destroy all marks attached to an object via connector */ +void fsnotify_destroy_marks(fsnotify_connp_t *connp) { struct fsnotify_mark_connector *conn; struct fsnotify_mark *mark, *old_mark = NULL; diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index b38964a7a521..e7cd7fc6e3cf 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -268,6 +268,8 @@ struct fsnotify_mark_connector { struct hlist_head list; }; +typedef struct fsnotify_mark_connector __rcu *fsnotify_connp_t; + /* * A mark is simply an object attached to an in core inode which allows an * fsnotify listener to indicate they are either no longer interested in events @@ -394,9 +396,8 @@ extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn); extern void fsnotify_init_mark(struct fsnotify_mark *mark, struct fsnotify_group *group); /* Find mark belonging to given group in the list of marks */ -extern struct fsnotify_mark *fsnotify_find_mark( - struct fsnotify_mark_connector __rcu **connp, - struct fsnotify_group *group); +extern struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp, + struct fsnotify_group *group); /* attach the mark to the inode or vfsmount */ extern int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode, struct vfsmount *mnt, int allow_dups);
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. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fsnotify.h | 4 ++-- fs/notify/mark.c | 20 +++++++++----------- include/linux/fsnotify_backend.h | 7 ++++--- 3 files changed, 15 insertions(+), 16 deletions(-)