diff mbox

[v2,05/20] fsnotify: use type id to identify connector object type

Message ID 1522934301-6520-6-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
An fsnotify_mark_connector is referencing a single type of object
(either inode or vfsmount). Instead of storing a type mask in
connector->flags, store a single type id in connector->type to
identify the type of object.

When a connector object is detached from the object, its type is set
to FSNOTIFY_OBJ_TYPE_DETACHED and this object is not going to be
reused.

The function fsnotify_clear_marks_by_group() is the only place where
type mask was used, so use type flags instead of type id to this
function.

This change is going to be more convenient when adding a new object
type (super block).

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fdinfo.c               |  6 +++---
 fs/notify/group.c                |  2 +-
 fs/notify/mark.c                 | 29 ++++++++++++++---------------
 include/linux/fsnotify_backend.h | 21 ++++++++++++++-------
 4 files changed, 32 insertions(+), 26 deletions(-)

Comments

Jan Kara April 17, 2018, 4:26 p.m. UTC | #1
On Thu 05-04-18 16:18:06, Amir Goldstein wrote:
> An fsnotify_mark_connector is referencing a single type of object
> (either inode or vfsmount). Instead of storing a type mask in
> connector->flags, store a single type id in connector->type to
> identify the type of object.
> 
> When a connector object is detached from the object, its type is set
> to FSNOTIFY_OBJ_TYPE_DETACHED and this object is not going to be
> reused.
> 
> The function fsnotify_clear_marks_by_group() is the only place where
> type mask was used, so use type flags instead of type id to this
> function.
> 
> This change is going to be more convenient when adding a new object
> type (super block).
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Just one nit below:

> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
> index 759ba9113ec4..9c690eb692a2 100644
> --- a/include/linux/fsnotify_backend.h
> +++ b/include/linux/fsnotify_backend.h
> @@ -201,6 +201,17 @@ struct fsnotify_group {
>  #define FSNOTIFY_EVENT_PATH	1
>  #define FSNOTIFY_EVENT_INODE	2
>  
> +enum fsnotify_obj_type {
> +	FSNOTIFY_OBJ_TYPE_INODE,
> +	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
> +	FSNOTIFY_OBJ_TYPE_MAX,

This would be better named FSNOTIFY_OBJ_TYPE_COUNT. _MAX suffix suggests
this is still a valid type. Or do you indeed want to treat TYPE_DETACHED as
a regular type?

								Honza

> +	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_MAX
> +};
Amir Goldstein April 17, 2018, 5:45 p.m. UTC | #2
On Tue, Apr 17, 2018 at 7:26 PM, Jan Kara <jack@suse.cz> wrote:
> On Thu 05-04-18 16:18:06, Amir Goldstein wrote:
>> An fsnotify_mark_connector is referencing a single type of object
>> (either inode or vfsmount). Instead of storing a type mask in
>> connector->flags, store a single type id in connector->type to
>> identify the type of object.
>>
>> When a connector object is detached from the object, its type is set
>> to FSNOTIFY_OBJ_TYPE_DETACHED and this object is not going to be
>> reused.
>>
>> The function fsnotify_clear_marks_by_group() is the only place where
>> type mask was used, so use type flags instead of type id to this
>> function.
>>
>> This change is going to be more convenient when adding a new object
>> type (super block).
>>
>> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> Just one nit below:
>
>> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
>> index 759ba9113ec4..9c690eb692a2 100644
>> --- a/include/linux/fsnotify_backend.h
>> +++ b/include/linux/fsnotify_backend.h
>> @@ -201,6 +201,17 @@ struct fsnotify_group {
>>  #define FSNOTIFY_EVENT_PATH  1
>>  #define FSNOTIFY_EVENT_INODE 2
>>
>> +enum fsnotify_obj_type {
>> +     FSNOTIFY_OBJ_TYPE_INODE,
>> +     FSNOTIFY_OBJ_TYPE_VFSMOUNT,
>> +     FSNOTIFY_OBJ_TYPE_MAX,
>
> This would be better named FSNOTIFY_OBJ_TYPE_COUNT. _MAX suffix suggests
> this is still a valid type. Or do you indeed want to treat TYPE_DETACHED as
> a regular type?
>

No. _COUNT is good. _DETACHED is never iterated when iterating by type.

Thanks,
Amir.
diff mbox

Patch

diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c
index d478629c728b..10aac1942c9f 100644
--- a/fs/notify/fdinfo.c
+++ b/fs/notify/fdinfo.c
@@ -77,7 +77,7 @@  static void inotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 	struct inotify_inode_mark *inode_mark;
 	struct inode *inode;
 
-	if (!(mark->connector->flags & FSNOTIFY_OBJ_TYPE_INODE))
+	if (mark->connector->type != FSNOTIFY_OBJ_TYPE_INODE)
 		return;
 
 	inode_mark = container_of(mark, struct inotify_inode_mark, fsn_mark);
@@ -116,7 +116,7 @@  static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 	if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)
 		mflags |= FAN_MARK_IGNORED_SURV_MODIFY;
 
-	if (mark->connector->flags & FSNOTIFY_OBJ_TYPE_INODE) {
+	if (mark->connector->type == FSNOTIFY_OBJ_TYPE_INODE) {
 		inode = igrab(mark->connector->inode);
 		if (!inode)
 			return;
@@ -126,7 +126,7 @@  static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark)
 		show_mark_fhandle(m, inode);
 		seq_putc(m, '\n');
 		iput(inode);
-	} else if (mark->connector->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
+	} else if (mark->connector->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
 		struct mount *mnt = real_mount(mark->connector->mnt);
 
 		seq_printf(m, "fanotify mnt_id:%x mflags:%x mask:%x ignored_mask:%x\n",
diff --git a/fs/notify/group.c b/fs/notify/group.c
index b7a4b6a69efa..aa5468f23e45 100644
--- a/fs/notify/group.c
+++ b/fs/notify/group.c
@@ -67,7 +67,7 @@  void fsnotify_destroy_group(struct fsnotify_group *group)
 	fsnotify_group_stop_queueing(group);
 
 	/* Clear all marks for this group and queue them for destruction */
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_ALL_TYPES);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_ALL_TYPES_MASK);
 
 	/*
 	 * Some marks can still be pinned when waiting for response from
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index e9191b416434..ef44808b28ca 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -119,9 +119,9 @@  static void __fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 		if (mark->flags & FSNOTIFY_MARK_FLAG_ATTACHED)
 			new_mask |= mark->mask;
 	}
-	if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE)
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
 		conn->inode->i_fsnotify_mask = new_mask;
-	else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+	else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT)
 		real_mount(conn->mnt)->mnt_fsnotify_mask = new_mask;
 }
 
@@ -139,7 +139,7 @@  void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
 	spin_lock(&conn->lock);
 	__fsnotify_recalc_mask(conn);
 	spin_unlock(&conn->lock);
-	if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE)
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
 		__fsnotify_update_child_dentry_flags(conn->inode);
 }
 
@@ -166,18 +166,18 @@  static struct inode *fsnotify_detach_connector_from_object(
 {
 	struct inode *inode = NULL;
 
-	if (conn->flags & FSNOTIFY_OBJ_TYPE_INODE) {
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
 		inode = conn->inode;
 		rcu_assign_pointer(inode->i_fsnotify_marks, NULL);
 		inode->i_fsnotify_mask = 0;
 		conn->inode = NULL;
-		conn->flags &= ~FSNOTIFY_OBJ_TYPE_INODE;
-	} else if (conn->flags & FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
+		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
+	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
 		rcu_assign_pointer(real_mount(conn->mnt)->mnt_fsnotify_marks,
 				   NULL);
 		real_mount(conn->mnt)->mnt_fsnotify_mask = 0;
 		conn->mnt = NULL;
-		conn->flags &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+		conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
 	}
 
 	return inode;
@@ -442,10 +442,10 @@  static int fsnotify_attach_connector_to_object(
 	spin_lock_init(&conn->lock);
 	INIT_HLIST_HEAD(&conn->list);
 	if (inode) {
-		conn->flags = FSNOTIFY_OBJ_TYPE_INODE;
+		conn->type = FSNOTIFY_OBJ_TYPE_INODE;
 		conn->inode = igrab(inode);
 	} else {
-		conn->flags = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
+		conn->type = FSNOTIFY_OBJ_TYPE_VFSMOUNT;
 		conn->mnt = mnt;
 	}
 	/*
@@ -479,8 +479,7 @@  static struct fsnotify_mark_connector *fsnotify_grab_connector(
 	if (!conn)
 		goto out;
 	spin_lock(&conn->lock);
-	if (!(conn->flags & (FSNOTIFY_OBJ_TYPE_INODE |
-			     FSNOTIFY_OBJ_TYPE_VFSMOUNT))) {
+	if (conn->type == FSNOTIFY_OBJ_TYPE_DETACHED) {
 		spin_unlock(&conn->lock);
 		srcu_read_unlock(&fsnotify_mark_srcu, idx);
 		return NULL;
@@ -646,16 +645,16 @@  struct fsnotify_mark *fsnotify_find_mark(
 	return NULL;
 }
 
-/* Clear any marks in a group with given type */
+/* Clear any marks in a group with given type mask */
 void fsnotify_clear_marks_by_group(struct fsnotify_group *group,
-				   unsigned int type)
+				   unsigned int type_mask)
 {
 	struct fsnotify_mark *lmark, *mark;
 	LIST_HEAD(to_free);
 	struct list_head *head = &to_free;
 
 	/* Skip selection step if we want to clear all marks. */
-	if (type == FSNOTIFY_OBJ_ALL_TYPES) {
+	if (type_mask == FSNOTIFY_OBJ_ALL_TYPES_MASK) {
 		head = &group->marks_list;
 		goto clear;
 	}
@@ -670,7 +669,7 @@  void fsnotify_clear_marks_by_group(struct fsnotify_group *group,
 	 */
 	mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
 	list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
-		if (mark->connector->flags & type)
+		if ((1U << mark->connector->type) & type_mask)
 			list_move(&mark->g_list, &to_free);
 	}
 	mutex_unlock(&group->mark_mutex);
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 759ba9113ec4..9c690eb692a2 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -201,6 +201,17 @@  struct fsnotify_group {
 #define FSNOTIFY_EVENT_PATH	1
 #define FSNOTIFY_EVENT_INODE	2
 
+enum fsnotify_obj_type {
+	FSNOTIFY_OBJ_TYPE_INODE,
+	FSNOTIFY_OBJ_TYPE_VFSMOUNT,
+	FSNOTIFY_OBJ_TYPE_MAX,
+	FSNOTIFY_OBJ_TYPE_DETACHED = FSNOTIFY_OBJ_TYPE_MAX
+};
+
+#define FSNOTIFY_OBJ_TYPE_INODE_FL	(1U << FSNOTIFY_OBJ_TYPE_INODE)
+#define FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL	(1U << FSNOTIFY_OBJ_TYPE_VFSMOUNT)
+#define FSNOTIFY_OBJ_ALL_TYPES_MASK	((1U << FSNOTIFY_OBJ_TYPE_MAX) - 1)
+
 /*
  * Inode / vfsmount point to this structure which tracks all marks attached to
  * the inode / vfsmount. The reference to inode / vfsmount is held by this
@@ -209,11 +220,7 @@  struct fsnotify_group {
  */
 struct fsnotify_mark_connector {
 	spinlock_t lock;
-#define FSNOTIFY_OBJ_TYPE_INODE		0x01
-#define FSNOTIFY_OBJ_TYPE_VFSMOUNT	0x02
-#define FSNOTIFY_OBJ_ALL_TYPES		(FSNOTIFY_OBJ_TYPE_INODE | \
-					 FSNOTIFY_OBJ_TYPE_VFSMOUNT)
-	unsigned int flags;	/* Type of object [lock] */
+	unsigned int type;	/* Type of object [lock] */
 	union {	/* Object pointer [lock] */
 		struct inode *inode;
 		struct vfsmount *mnt;
@@ -365,12 +372,12 @@  extern void fsnotify_clear_marks_by_group(struct fsnotify_group *group, unsigned
 /* run all the marks in a group, and clear all of the vfsmount marks */
 static inline void fsnotify_clear_vfsmount_marks_by_group(struct fsnotify_group *group)
 {
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_VFSMOUNT);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL);
 }
 /* run all the marks in a group, and clear all of the inode marks */
 static inline void fsnotify_clear_inode_marks_by_group(struct fsnotify_group *group)
 {
-	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_INODE);
+	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_TYPE_INODE_FL);
 }
 extern void fsnotify_get_mark(struct fsnotify_mark *mark);
 extern void fsnotify_put_mark(struct fsnotify_mark *mark);