diff mbox

[v2,14/20] fsnotify: pass object and object type to fsnotify_add_mark()

Message ID 1522934301-6520-15-git-send-email-amir73il@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Amir Goldstein April 5, 2018, 1:18 p.m. UTC 
Instead of passing inode and vfsmount arguments to fsnotify_add_mark()
and its _locked variant, pass an abstract fsnotify_obj and the object
type.

This is going to be used for adding a mark to a new type of object
(super block object).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fanotify/fanotify_user.c | 16 ++++++++-------
 fs/notify/fsnotify.h               | 11 ++++++++++
 fs/notify/mark.c                   | 42 +++++++++++++++-----------------------
 include/linux/fsnotify_backend.h   | 18 +++++++++++-----
 4 files changed, 50 insertions(+), 37 deletions(-)

Comments

Jan Kara April 19, 2018, 12:23 p.m. UTC | #1 
On Thu 05-04-18 16:18:15, Amir Goldstein wrote:
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 112f54dceeef..ea6d97f5fc3b 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -437,9 +437,9 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
>  }
>  
>  static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
> -					       struct inode *inode,
> -					       struct vfsmount *mnt)
> +					       unsigned int type)
>  {
> +	struct inode *inode = NULL;
>  	struct fsnotify_mark_connector *conn;
>  
>  	conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
> @@ -447,13 +447,11 @@ static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
>  		return -ENOMEM;
>  	spin_lock_init(&conn->lock);
>  	INIT_HLIST_HEAD(&conn->list);
> -	if (inode) {
> -		conn->type = FSNOTIFY_OBJ_TYPE_INODE;
> -		conn->inode = igrab(inode);
> -	} else {
> -		conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
> -		conn->mnt = mnt;
> -	}
> +	conn->type = type;
> +	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
> +		inode = conn->inode = igrab(fsnotify_inode(obj));
> +	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
> +		conn->mnt = fsnotify_vfsmount(obj);


How about making connector point to fsnotify_obj and then get real inode /
mount pointers only in those few places which need them (basically only for
grabbing inode references and dentry flag propagation AFAIR)? That should
make the code even less aware of different object types.

Also fsnotify_inode() and fsnotify_vfsmount() seem like too generic names.
I'd call them fsnotify_obj_inode() and fsnotify_obj_vfsmount().

								Honza
Amir Goldstein April 19, 2018, 2:28 p.m. UTC | #2 
On Thu, Apr 19, 2018 at 3:23 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:15, Amir Goldstein wrote:
>> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
>> index 112f54dceeef..ea6d97f5fc3b 100644
>> --- a/fs/notify/mark.c
>> +++ b/fs/notify/mark.c
>> @@ -437,9 +437,9 @@ int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
>>  }
>>
>>  static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
>> -                                            struct inode *inode,
>> -                                            struct vfsmount *mnt)
>> +                                            unsigned int type)
>>  {
>> +     struct inode *inode = NULL;
>>       struct fsnotify_mark_connector *conn;
>>
>>       conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
>> @@ -447,13 +447,11 @@ static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
>>               return -ENOMEM;
>>       spin_lock_init(&conn->lock);
>>       INIT_HLIST_HEAD(&conn->list);
>> -     if (inode) {
>> -             conn->type = FSNOTIFY_OBJ_TYPE_INODE;
>> -             conn->inode = igrab(inode);
>> -     } else {
>> -             conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
>> -             conn->mnt = mnt;
>> -     }
>> +     conn->type = type;
>> +     if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
>> +             inode = conn->inode = igrab(fsnotify_inode(obj));
>> +     else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
>> +             conn->mnt = fsnotify_vfsmount(obj);
>
>
> How about making connector point to fsnotify_obj and then get real inode /
> mount pointers only in those few places which need them (basically only for
> grabbing inode references and dentry flag propagation AFAIR)? That should
> make the code even less aware of different object types.
>

Yes, I thought of doing that. I will add it in another patch, to limit
the changes of
this one.

> Also fsnotify_inode() and fsnotify_vfsmount() seem like too generic names.
> I'd call them fsnotify_obj_inode() and fsnotify_obj_vfsmount().
>

OK.

Thanks,
Amir.
diff mbox

Patch

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 8da861cb3f5c..ecd3539587cc 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -614,8 +614,8 @@  static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark,
 }
 
 static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
-						   struct inode *inode,
-						   struct vfsmount *mnt)
+						   struct fsnotify_obj *obj,
+						   unsigned int type)
 {
 	struct fsnotify_mark *mark;
 	int ret;
@@ -628,7 +628,7 @@  static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group,
 		return ERR_PTR(-ENOMEM);
 
 	fsnotify_init_mark(mark, group);
-	ret = fsnotify_add_mark_locked(mark, inode, mnt, 0);
+	ret = fsnotify_add_mark_locked(mark, obj, type, 0);
 	if (ret) {
 		fsnotify_put_mark(mark);
 		return ERR_PTR(ret);
@@ -642,13 +642,14 @@  static int fanotify_add_vfsmount_mark(struct fsnotify_group *group,
 				      struct vfsmount *mnt, __u32 mask,
 				      unsigned int flags)
 {
+	struct fsnotify_obj *obj = &real_mount(mnt)->mnt_fsnotify;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&real_mount(mnt)->mnt_fsnotify, group);
+	fsn_mark = fsnotify_find_mark(obj, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, NULL, mnt);
+		fsn_mark = fanotify_add_new_mark(group, obj, FSNOTIFY_OBJ_TYPE_VFSMOUNT);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
@@ -667,6 +668,7 @@  static int fanotify_add_inode_mark(struct fsnotify_group *group,
 				   struct inode *inode, __u32 mask,
 				   unsigned int flags)
 {
+	struct fsnotify_obj *obj = &inode->i_fsnotify;
 	struct fsnotify_mark *fsn_mark;
 	__u32 added;
 
@@ -683,9 +685,9 @@  static int fanotify_add_inode_mark(struct fsnotify_group *group,
 		return 0;
 
 	mutex_lock(&group->mark_mutex);
-	fsn_mark = fsnotify_find_mark(&inode->i_fsnotify, group);
+	fsn_mark = fsnotify_find_mark(obj, group);
 	if (!fsn_mark) {
-		fsn_mark = fanotify_add_new_mark(group, inode, NULL);
+		fsn_mark = fanotify_add_new_mark(group, obj, FSNOTIFY_OBJ_TYPE_INODE);
 		if (IS_ERR(fsn_mark)) {
 			mutex_unlock(&group->mark_mutex);
 			return PTR_ERR(fsn_mark);
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index 6459481c964f..2a3fe62e2bf6 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -44,4 +44,15 @@  extern void __fsnotify_update_child_dentry_flags(struct inode *inode);
 extern struct fsnotify_event_holder *fsnotify_alloc_event_holder(void);
 extern void fsnotify_destroy_event_holder(struct fsnotify_event_holder *holder);
 
+static inline struct inode *fsnotify_inode(struct fsnotify_obj *obj)
+{
+	return container_of(obj, struct inode, i_fsnotify);
+}
+
+static inline struct vfsmount *fsnotify_vfsmount(struct fsnotify_obj *obj)
+{
+	return &(container_of(obj, struct mount, mnt_fsnotify))->mnt;
+}
+
+
 #endif	/* __FS_NOTIFY_FSNOTIFY_H_ */
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 112f54dceeef..ea6d97f5fc3b 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -437,9 +437,9 @@  int fsnotify_compare_groups(struct fsnotify_group *a, struct fsnotify_group *b)
 }
 
 static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
-					       struct inode *inode,
-					       struct vfsmount *mnt)
+					       unsigned int type)
 {
+	struct inode *inode = NULL;
 	struct fsnotify_mark_connector *conn;
 
 	conn = kmem_cache_alloc(fsnotify_mark_connector_cachep, GFP_KERNEL);
@@ -447,13 +447,11 @@  static int fsnotify_attach_connector_to_object(struct fsnotify_obj *obj,
 		return -ENOMEM;
 	spin_lock_init(&conn->lock);
 	INIT_HLIST_HEAD(&conn->list);
-	if (inode) {
-		conn->type = FSNOTIFY_OBJ_TYPE_INODE;
-		conn->inode = igrab(inode);
-	} else {
-		conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
-		conn->mnt = mnt;
-	}
+	conn->type = type;
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
+		inode = conn->inode = igrab(fsnotify_inode(obj));
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+		conn->mnt = fsnotify_vfsmount(obj);
 	/*
 	 * cmpxchg() provides the barrier so that readers of obj->marks can see
 	 * only initialized structure
@@ -502,27 +500,22 @@  static struct fsnotify_mark_connector *fsnotify_grab_connector(
  * priority, highest number first, and then by the group's location in memory.
  */
 static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
-				  struct inode *inode, struct vfsmount *mnt,
+				  struct fsnotify_obj *obj, unsigned int type,
 				  int allow_dups)
 {
 	struct fsnotify_mark *lmark, *last = NULL;
 	struct fsnotify_mark_connector *conn;
-	struct fsnotify_obj *obj;
 	int cmp;
 	int err = 0;
 
-	if (WARN_ON(!inode && !mnt))
+	if (WARN_ON(!fsnotify_valid_obj_type(type)))
 		return -EINVAL;
-	if (inode)
-		obj = &inode->i_fsnotify;
-	else
-		obj = &real_mount(mnt)->mnt_fsnotify;
 restart:
 	spin_lock(&mark->lock);
 	conn = fsnotify_grab_connector(obj);
 	if (!conn) {
 		spin_unlock(&mark->lock);
-		err = fsnotify_attach_connector_to_object(obj, inode, mnt);
+		err = fsnotify_attach_connector_to_object(obj, type);
 		if (err)
 			return err;
 		goto restart;
@@ -568,14 +561,13 @@  static int fsnotify_add_mark_list(struct fsnotify_mark *mark,
  * These marks may be used for the fsnotify backend to determine which
  * event types should be delivered to which group.
  */
-int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
-			     struct vfsmount *mnt, int allow_dups)
+int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
+			     struct fsnotify_obj *obj, unsigned int type,
+			     int allow_dups)
 {
 	struct fsnotify_group *group = mark->group;
 	int ret = 0;
 
-	BUG_ON(inode && mnt);
-	BUG_ON(!inode && !mnt);
 	BUG_ON(!mutex_is_locked(&group->mark_mutex));
 
 	/*
@@ -592,7 +584,7 @@  int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
 	fsnotify_get_mark(mark); /* for g_list */
 	spin_unlock(&mark->lock);
 
-	ret = fsnotify_add_mark_list(mark, inode, mnt, allow_dups);
+	ret = fsnotify_add_mark_list(mark, obj, type, allow_dups);
 	if (ret)
 		goto err;
 
@@ -612,14 +604,14 @@  int fsnotify_add_mark_locked(struct fsnotify_mark *mark, struct inode *inode,
 	return ret;
 }
 
-int fsnotify_add_mark(struct fsnotify_mark *mark, struct inode *inode,
-		      struct vfsmount *mnt, int allow_dups)
+int fsnotify_add_mark(struct fsnotify_mark *mark, struct fsnotify_obj *obj,
+		      unsigned int type, int allow_dups)
 {
 	int ret;
 	struct fsnotify_group *group = mark->group;
 
 	mutex_lock(&group->mark_mutex);
-	ret = fsnotify_add_mark_locked(mark, inode, mnt, allow_dups);
+	ret = fsnotify_add_mark_locked(mark, obj, type, allow_dups);
 	mutex_unlock(&group->mark_mutex);
 	return ret;
 }
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 86ea950561a8..4a034e6832ea 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -211,6 +211,11 @@  enum fsnotify_obj_type {
 #define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 #define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
 
+static inline bool fsnotify_valid_obj_type(unsigned int type)
+{
+	return (type < FSNOTIFY_OBJ_TYPE_MAX);
+}
+
 struct fsnotify_iter_info {
 	struct fsnotify_mark *marks[FSNOTIFY_OBJ_TYPE_MAX];
 	unsigned int type_mask;
@@ -403,23 +408,26 @@  extern void fsnotify_init_mark(struct fsnotify_mark *mark,
 extern struct fsnotify_mark *fsnotify_find_mark(struct fsnotify_obj *obj,
 						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);
+extern int fsnotify_add_mark(struct fsnotify_mark *mark,
+			     struct fsnotify_obj *obj, unsigned int type,
+			     int allow_dups);
 extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
-				    struct inode *inode, struct vfsmount *mnt,
+				    struct fsnotify_obj *obj, unsigned int type,
 				    int allow_dups);
 /* attach the mark to the inode */
 static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark,
 					  struct inode *inode,
 					  int allow_dups)
 {
-	return fsnotify_add_mark(mark, inode, NULL, allow_dups);
+	return fsnotify_add_mark(mark, &inode->i_fsnotify,
+				 FSNOTIFY_OBJ_TYPE_INODE, allow_dups);
 }
 static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark,
 						 struct inode *inode,
 						 int allow_dups)
 {
-	return fsnotify_add_mark_locked(mark, inode, NULL, allow_dups);
+	return fsnotify_add_mark_locked(mark, &inode->i_fsnotify,
+					FSNOTIFY_OBJ_TYPE_INODE, allow_dups);
 }
 /* given a group and a mark, flag mark to be freed when all references are dropped */
 extern void fsnotify_destroy_mark(struct fsnotify_mark *mark,