linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] Performance optimization for no fsnotify marks
@ 2021-08-10 15:12 Amir Goldstein
  2021-08-10 15:12 ` [PATCH v2 1/4] fsnotify: replace igrab() with ihold() on attach connector Amir Goldstein
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Amir Goldstein @ 2021-08-10 15:12 UTC (permalink / raw)
  To: Jan Kara; +Cc: Matthew Bobrowski, linux-fsdevel

Hi Jan,

Following v2 addresses review comments from v1 [1]

[1] https://lore.kernel.org/linux-fsdevel/20210803180344.2398374-1-amir73il@gmail.com/

Changes since v1:
- Rebase on 5.14-rc5 + pidfd patches
- Added RVB
- Helper to get connector's sb (Matthew)
- Fix deadlock bug on umount (Jan)

Amir Goldstein (4):
  fsnotify: replace igrab() with ihold() on attach connector
  fsnotify: count s_fsnotify_inode_refs for attached connectors
  fsnotify: count all objects with attached connectors
  fsnotify: optimize the case of no marks of any type

 fs/notify/fsnotify.c     |  6 ++---
 fs/notify/fsnotify.h     | 15 ++++++++++++
 fs/notify/mark.c         | 52 ++++++++++++++++++++++++++++++----------
 include/linux/fs.h       |  4 ++--
 include/linux/fsnotify.h |  9 +++++++
 5 files changed, 69 insertions(+), 17 deletions(-)

-- 
2.32.0


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 1/4] fsnotify: replace igrab() with ihold() on attach connector
  2021-08-10 15:12 [PATCH v2 0/4] Performance optimization for no fsnotify marks Amir Goldstein
@ 2021-08-10 15:12 ` Amir Goldstein
  2021-08-10 15:12 ` [PATCH v2 2/4] fsnotify: count s_fsnotify_inode_refs for attached connectors Amir Goldstein
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2021-08-10 15:12 UTC (permalink / raw)
  To: Jan Kara; +Cc: Matthew Bobrowski, linux-fsdevel, Matthew Bobrowski

We must have a reference on inode, so ihold is cheaper.

Reviewed-by: Matthew Bobrowski <repnop@google.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/mark.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index d32ab349db74..80459db58f63 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -493,8 +493,11 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 		conn->fsid.val[0] = conn->fsid.val[1] = 0;
 		conn->flags = 0;
 	}
-	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE)
-		inode = igrab(fsnotify_conn_inode(conn));
+	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
+		inode = fsnotify_conn_inode(conn);
+		ihold(inode);
+	}
+
 	/*
 	 * cmpxchg() provides the barrier so that readers of *connp can see
 	 * only initialized structure
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 2/4] fsnotify: count s_fsnotify_inode_refs for attached connectors
  2021-08-10 15:12 [PATCH v2 0/4] Performance optimization for no fsnotify marks Amir Goldstein
  2021-08-10 15:12 ` [PATCH v2 1/4] fsnotify: replace igrab() with ihold() on attach connector Amir Goldstein
@ 2021-08-10 15:12 ` Amir Goldstein
  2021-08-10 15:12 ` [PATCH v2 3/4] fsnotify: count all objects with " Amir Goldstein
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2021-08-10 15:12 UTC (permalink / raw)
  To: Jan Kara; +Cc: Matthew Bobrowski, linux-fsdevel, Matthew Bobrowski

Instead of incrementing s_fsnotify_inode_refs when detaching connector
from inode, increment it earlier when attaching connector to inode.
Next patch is going to use s_fsnotify_inode_refs to count all objects
with attached connectors.

Reviewed-by: Matthew Bobrowski <repnop@google.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/mark.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 80459db58f63..2d8c46e1167d 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -169,6 +169,21 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work)
 	}
 }
 
+static void fsnotify_get_inode_ref(struct inode *inode)
+{
+	ihold(inode);
+	atomic_long_inc(&inode->i_sb->s_fsnotify_inode_refs);
+}
+
+static void fsnotify_put_inode_ref(struct inode *inode)
+{
+	struct super_block *sb = inode->i_sb;
+
+	iput(inode);
+	if (atomic_long_dec_and_test(&sb->s_fsnotify_inode_refs))
+		wake_up_var(&sb->s_fsnotify_inode_refs);
+}
+
 static void *fsnotify_detach_connector_from_object(
 					struct fsnotify_mark_connector *conn,
 					unsigned int *type)
@@ -182,7 +197,6 @@ static void *fsnotify_detach_connector_from_object(
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
 		inode = fsnotify_conn_inode(conn);
 		inode->i_fsnotify_mask = 0;
-		atomic_long_inc(&inode->i_sb->s_fsnotify_inode_refs);
 	} else if (conn->type == FSNOTIFY_OBJ_TYPE_VFSMOUNT) {
 		fsnotify_conn_mount(conn)->mnt_fsnotify_mask = 0;
 	} else if (conn->type == FSNOTIFY_OBJ_TYPE_SB) {
@@ -209,19 +223,12 @@ static void fsnotify_final_mark_destroy(struct fsnotify_mark *mark)
 /* Drop object reference originally held by a connector */
 static void fsnotify_drop_object(unsigned int type, void *objp)
 {
-	struct inode *inode;
-	struct super_block *sb;
-
 	if (!objp)
 		return;
 	/* Currently only inode references are passed to be dropped */
 	if (WARN_ON_ONCE(type != FSNOTIFY_OBJ_TYPE_INODE))
 		return;
-	inode = objp;
-	sb = inode->i_sb;
-	iput(inode);
-	if (atomic_long_dec_and_test(&sb->s_fsnotify_inode_refs))
-		wake_up_var(&sb->s_fsnotify_inode_refs);
+	fsnotify_put_inode_ref(objp);
 }
 
 void fsnotify_put_mark(struct fsnotify_mark *mark)
@@ -495,7 +502,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	}
 	if (conn->type == FSNOTIFY_OBJ_TYPE_INODE) {
 		inode = fsnotify_conn_inode(conn);
-		ihold(inode);
+		fsnotify_get_inode_ref(inode);
 	}
 
 	/*
@@ -505,7 +512,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 	if (cmpxchg(connp, NULL, conn)) {
 		/* Someone else created list structure for us */
 		if (inode)
-			iput(inode);
+			fsnotify_put_inode_ref(inode);
 		kmem_cache_free(fsnotify_mark_connector_cachep, conn);
 	}
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 3/4] fsnotify: count all objects with attached connectors
  2021-08-10 15:12 [PATCH v2 0/4] Performance optimization for no fsnotify marks Amir Goldstein
  2021-08-10 15:12 ` [PATCH v2 1/4] fsnotify: replace igrab() with ihold() on attach connector Amir Goldstein
  2021-08-10 15:12 ` [PATCH v2 2/4] fsnotify: count s_fsnotify_inode_refs for attached connectors Amir Goldstein
@ 2021-08-10 15:12 ` Amir Goldstein
  2021-08-10 21:49   ` Matthew Bobrowski
  2021-08-11 11:37   ` Jan Kara
  2021-08-10 15:12 ` [PATCH v2 4/4] fsnotify: optimize the case of no marks of any type Amir Goldstein
  2021-08-11 12:02 ` [PATCH v2 0/4] Performance optimization for no fsnotify marks Jan Kara
  4 siblings, 2 replies; 9+ messages in thread
From: Amir Goldstein @ 2021-08-10 15:12 UTC (permalink / raw)
  To: Jan Kara; +Cc: Matthew Bobrowski, linux-fsdevel

Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all
objects with attached connectors, not only inodes with attached
connectors.

This will be used to optimize fsnotify() calls on sb without any
type of marks.

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/notify/fsnotify.c |  6 +++---
 fs/notify/fsnotify.h | 15 +++++++++++++++
 fs/notify/mark.c     | 24 +++++++++++++++++++++---
 include/linux/fs.h   |  4 ++--
 4 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
index 30d422b8c0fc..963e6ce75b96 100644
--- a/fs/notify/fsnotify.c
+++ b/fs/notify/fsnotify.c
@@ -87,15 +87,15 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
 
 	if (iput_inode)
 		iput(iput_inode);
-	/* Wait for outstanding inode references from connectors */
-	wait_var_event(&sb->s_fsnotify_inode_refs,
-		       !atomic_long_read(&sb->s_fsnotify_inode_refs));
 }
 
 void fsnotify_sb_delete(struct super_block *sb)
 {
 	fsnotify_unmount_inodes(sb);
 	fsnotify_clear_marks_by_sb(sb);
+	/* Wait for outstanding object references from connectors */
+	wait_var_event(&sb->s_fsnotify_connectors,
+		       !atomic_long_read(&sb->s_fsnotify_connectors));
 }
 
 /*
diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
index ff2063ec6b0f..87d8a50ee803 100644
--- a/fs/notify/fsnotify.h
+++ b/fs/notify/fsnotify.h
@@ -27,6 +27,21 @@ static inline struct super_block *fsnotify_conn_sb(
 	return container_of(conn->obj, struct super_block, s_fsnotify_marks);
 }
 
+static inline struct super_block *fsnotify_connector_sb(
+				struct fsnotify_mark_connector *conn)
+{
+	switch (conn->type) {
+	case FSNOTIFY_OBJ_TYPE_INODE:
+		return fsnotify_conn_inode(conn)->i_sb;
+	case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
+		return fsnotify_conn_mount(conn)->mnt.mnt_sb;
+	case FSNOTIFY_OBJ_TYPE_SB:
+		return fsnotify_conn_sb(conn);
+	default:
+		return NULL;
+	}
+}
+
 /* destroy all events sitting in this groups notification queue */
 extern void fsnotify_flush_notify(struct fsnotify_group *group);
 
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index 2d8c46e1167d..95006d1d29ab 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -172,7 +172,7 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work)
 static void fsnotify_get_inode_ref(struct inode *inode)
 {
 	ihold(inode);
-	atomic_long_inc(&inode->i_sb->s_fsnotify_inode_refs);
+	atomic_long_inc(&inode->i_sb->s_fsnotify_connectors);
 }
 
 static void fsnotify_put_inode_ref(struct inode *inode)
@@ -180,8 +180,24 @@ static void fsnotify_put_inode_ref(struct inode *inode)
 	struct super_block *sb = inode->i_sb;
 
 	iput(inode);
-	if (atomic_long_dec_and_test(&sb->s_fsnotify_inode_refs))
-		wake_up_var(&sb->s_fsnotify_inode_refs);
+	if (atomic_long_dec_and_test(&sb->s_fsnotify_connectors))
+		wake_up_var(&sb->s_fsnotify_connectors);
+}
+
+static void fsnotify_get_sb_connectors(struct fsnotify_mark_connector *conn)
+{
+	struct super_block *sb = fsnotify_connector_sb(conn);
+
+	if (sb)
+		atomic_long_inc(&sb->s_fsnotify_connectors);
+}
+
+static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn)
+{
+	struct super_block *sb = fsnotify_connector_sb(conn);
+
+	if (sb && atomic_long_dec_and_test(&sb->s_fsnotify_connectors))
+		wake_up_var(&sb->s_fsnotify_connectors);
 }
 
 static void *fsnotify_detach_connector_from_object(
@@ -203,6 +219,7 @@ static void *fsnotify_detach_connector_from_object(
 		fsnotify_conn_sb(conn)->s_fsnotify_mask = 0;
 	}
 
+	fsnotify_put_sb_connectors(conn);
 	rcu_assign_pointer(*(conn->obj), NULL);
 	conn->obj = NULL;
 	conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
@@ -504,6 +521,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
 		inode = fsnotify_conn_inode(conn);
 		fsnotify_get_inode_ref(inode);
 	}
+	fsnotify_get_sb_connectors(conn);
 
 	/*
 	 * cmpxchg() provides the barrier so that readers of *connp can see
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 640574294216..d48d2018dfa4 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1507,8 +1507,8 @@ struct super_block {
 	/* Number of inodes with nlink == 0 but still referenced */
 	atomic_long_t s_remove_count;
 
-	/* Pending fsnotify inode refs */
-	atomic_long_t s_fsnotify_inode_refs;
+	/* Number of inode/mount/sb objects that are being watched */
+	atomic_long_t s_fsnotify_connectors;
 
 	/* Being remounted read-only */
 	int s_readonly_remount;
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [PATCH v2 4/4] fsnotify: optimize the case of no marks of any type
  2021-08-10 15:12 [PATCH v2 0/4] Performance optimization for no fsnotify marks Amir Goldstein
                   ` (2 preceding siblings ...)
  2021-08-10 15:12 ` [PATCH v2 3/4] fsnotify: count all objects with " Amir Goldstein
@ 2021-08-10 15:12 ` Amir Goldstein
  2021-08-11 12:02 ` [PATCH v2 0/4] Performance optimization for no fsnotify marks Jan Kara
  4 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2021-08-10 15:12 UTC (permalink / raw)
  To: Jan Kara; +Cc: Matthew Bobrowski, linux-fsdevel, Matthew Bobrowski

Add a simple check in the inline helpers to avoid calling fsnotify()
and __fsnotify_parent() in case there are no marks of any type
(inode/sb/mount) for an inode's sb, so there can be no objects
of any type interested in the event.

Reviewed-by: Matthew Bobrowski <repnop@google.com>
Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 include/linux/fsnotify.h | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
index f8acddcf54fb..12d3a7d308ab 100644
--- a/include/linux/fsnotify.h
+++ b/include/linux/fsnotify.h
@@ -30,6 +30,9 @@ static inline void fsnotify_name(struct inode *dir, __u32 mask,
 				 struct inode *child,
 				 const struct qstr *name, u32 cookie)
 {
+	if (atomic_long_read(&dir->i_sb->s_fsnotify_connectors) == 0)
+		return;
+
 	fsnotify(mask, child, FSNOTIFY_EVENT_INODE, dir, name, NULL, cookie);
 }
 
@@ -41,6 +44,9 @@ static inline void fsnotify_dirent(struct inode *dir, struct dentry *dentry,
 
 static inline void fsnotify_inode(struct inode *inode, __u32 mask)
 {
+	if (atomic_long_read(&inode->i_sb->s_fsnotify_connectors) == 0)
+		return;
+
 	if (S_ISDIR(inode->i_mode))
 		mask |= FS_ISDIR;
 
@@ -53,6 +59,9 @@ static inline int fsnotify_parent(struct dentry *dentry, __u32 mask,
 {
 	struct inode *inode = d_inode(dentry);
 
+	if (atomic_long_read(&inode->i_sb->s_fsnotify_connectors) == 0)
+		return 0;
+
 	if (S_ISDIR(inode->i_mode)) {
 		mask |= FS_ISDIR;
 
-- 
2.32.0


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/4] fsnotify: count all objects with attached connectors
  2021-08-10 15:12 ` [PATCH v2 3/4] fsnotify: count all objects with " Amir Goldstein
@ 2021-08-10 21:49   ` Matthew Bobrowski
  2021-08-11 11:37   ` Jan Kara
  1 sibling, 0 replies; 9+ messages in thread
From: Matthew Bobrowski @ 2021-08-10 21:49 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, Matthew Bobrowski, linux-fsdevel

On Tue, Aug 10, 2021 at 06:12:19PM +0300, Amir Goldstein wrote:
> Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all
> objects with attached connectors, not only inodes with attached
> connectors.
> 
> This will be used to optimize fsnotify() calls on sb without any
> type of marks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

LGTM.

Reviewed-by: Matthew Bobrowski <repnop@google.com>

> ---
>  fs/notify/fsnotify.c |  6 +++---
>  fs/notify/fsnotify.h | 15 +++++++++++++++
>  fs/notify/mark.c     | 24 +++++++++++++++++++++---
>  include/linux/fs.h   |  4 ++--
>  4 files changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 30d422b8c0fc..963e6ce75b96 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -87,15 +87,15 @@ static void fsnotify_unmount_inodes(struct super_block *sb)
>  
>  	if (iput_inode)
>  		iput(iput_inode);
> -	/* Wait for outstanding inode references from connectors */
> -	wait_var_event(&sb->s_fsnotify_inode_refs,
> -		       !atomic_long_read(&sb->s_fsnotify_inode_refs));
>  }
>  
>  void fsnotify_sb_delete(struct super_block *sb)
>  {
>  	fsnotify_unmount_inodes(sb);
>  	fsnotify_clear_marks_by_sb(sb);
> +	/* Wait for outstanding object references from connectors */
> +	wait_var_event(&sb->s_fsnotify_connectors,
> +		       !atomic_long_read(&sb->s_fsnotify_connectors));
>  }
>  
>  /*
> diff --git a/fs/notify/fsnotify.h b/fs/notify/fsnotify.h
> index ff2063ec6b0f..87d8a50ee803 100644
> --- a/fs/notify/fsnotify.h
> +++ b/fs/notify/fsnotify.h
> @@ -27,6 +27,21 @@ static inline struct super_block *fsnotify_conn_sb(
>  	return container_of(conn->obj, struct super_block, s_fsnotify_marks);
>  }
>  
> +static inline struct super_block *fsnotify_connector_sb(
> +				struct fsnotify_mark_connector *conn)
> +{
> +	switch (conn->type) {
> +	case FSNOTIFY_OBJ_TYPE_INODE:
> +		return fsnotify_conn_inode(conn)->i_sb;
> +	case FSNOTIFY_OBJ_TYPE_VFSMOUNT:
> +		return fsnotify_conn_mount(conn)->mnt.mnt_sb;
> +	case FSNOTIFY_OBJ_TYPE_SB:
> +		return fsnotify_conn_sb(conn);
> +	default:
> +		return NULL;
> +	}
> +}
> +
>  /* destroy all events sitting in this groups notification queue */
>  extern void fsnotify_flush_notify(struct fsnotify_group *group);
>  
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index 2d8c46e1167d..95006d1d29ab 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -172,7 +172,7 @@ static void fsnotify_connector_destroy_workfn(struct work_struct *work)
>  static void fsnotify_get_inode_ref(struct inode *inode)
>  {
>  	ihold(inode);
> -	atomic_long_inc(&inode->i_sb->s_fsnotify_inode_refs);
> +	atomic_long_inc(&inode->i_sb->s_fsnotify_connectors);
>  }
>  
>  static void fsnotify_put_inode_ref(struct inode *inode)
> @@ -180,8 +180,24 @@ static void fsnotify_put_inode_ref(struct inode *inode)
>  	struct super_block *sb = inode->i_sb;
>  
>  	iput(inode);
> -	if (atomic_long_dec_and_test(&sb->s_fsnotify_inode_refs))
> -		wake_up_var(&sb->s_fsnotify_inode_refs);
> +	if (atomic_long_dec_and_test(&sb->s_fsnotify_connectors))
> +		wake_up_var(&sb->s_fsnotify_connectors);
> +}
> +
> +static void fsnotify_get_sb_connectors(struct fsnotify_mark_connector *conn)
> +{
> +	struct super_block *sb = fsnotify_connector_sb(conn);
> +
> +	if (sb)
> +		atomic_long_inc(&sb->s_fsnotify_connectors);
> +}
> +
> +static void fsnotify_put_sb_connectors(struct fsnotify_mark_connector *conn)
> +{
> +	struct super_block *sb = fsnotify_connector_sb(conn);
> +
> +	if (sb && atomic_long_dec_and_test(&sb->s_fsnotify_connectors))
> +		wake_up_var(&sb->s_fsnotify_connectors);
>  }
>  
>  static void *fsnotify_detach_connector_from_object(
> @@ -203,6 +219,7 @@ static void *fsnotify_detach_connector_from_object(
>  		fsnotify_conn_sb(conn)->s_fsnotify_mask = 0;
>  	}
>  
> +	fsnotify_put_sb_connectors(conn);
>  	rcu_assign_pointer(*(conn->obj), NULL);
>  	conn->obj = NULL;
>  	conn->type = FSNOTIFY_OBJ_TYPE_DETACHED;
> @@ -504,6 +521,7 @@ static int fsnotify_attach_connector_to_object(fsnotify_connp_t *connp,
>  		inode = fsnotify_conn_inode(conn);
>  		fsnotify_get_inode_ref(inode);
>  	}
> +	fsnotify_get_sb_connectors(conn);
>  
>  	/*
>  	 * cmpxchg() provides the barrier so that readers of *connp can see
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 640574294216..d48d2018dfa4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1507,8 +1507,8 @@ struct super_block {
>  	/* Number of inodes with nlink == 0 but still referenced */
>  	atomic_long_t s_remove_count;
>  
> -	/* Pending fsnotify inode refs */
> -	atomic_long_t s_fsnotify_inode_refs;
> +	/* Number of inode/mount/sb objects that are being watched */
> +	atomic_long_t s_fsnotify_connectors;
>  
>  	/* Being remounted read-only */
>  	int s_readonly_remount;
> -- 
> 2.32.0
> 
/M

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/4] fsnotify: count all objects with attached connectors
  2021-08-10 15:12 ` [PATCH v2 3/4] fsnotify: count all objects with " Amir Goldstein
  2021-08-10 21:49   ` Matthew Bobrowski
@ 2021-08-11 11:37   ` Jan Kara
  2021-08-11 13:59     ` Amir Goldstein
  1 sibling, 1 reply; 9+ messages in thread
From: Jan Kara @ 2021-08-11 11:37 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, Matthew Bobrowski, linux-fsdevel

On Tue 10-08-21 18:12:19, Amir Goldstein wrote:
> Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all
> objects with attached connectors, not only inodes with attached
> connectors.
> 
> This will be used to optimize fsnotify() calls on sb without any
> type of marks.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

... just a minor nit below ...

> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 640574294216..d48d2018dfa4 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1507,8 +1507,8 @@ struct super_block {
>  	/* Number of inodes with nlink == 0 but still referenced */
>  	atomic_long_t s_remove_count;
>  
> -	/* Pending fsnotify inode refs */
> -	atomic_long_t s_fsnotify_inode_refs;
> +	/* Number of inode/mount/sb objects that are being watched */
> +	atomic_long_t s_fsnotify_connectors;

I've realized inode watches will be double-accounted because we increment
s_fsnotify_connectors both when attaching a connector and when grabbing
inode reference. It doesn't really matter and avoiding this would require
special treatment of inode connectors (we need to decrement
s_fsnotify_connectors only after dropping inode reference). So I'll just
reflect this in the comment here so that we don't forget.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 0/4] Performance optimization for no fsnotify marks
  2021-08-10 15:12 [PATCH v2 0/4] Performance optimization for no fsnotify marks Amir Goldstein
                   ` (3 preceding siblings ...)
  2021-08-10 15:12 ` [PATCH v2 4/4] fsnotify: optimize the case of no marks of any type Amir Goldstein
@ 2021-08-11 12:02 ` Jan Kara
  4 siblings, 0 replies; 9+ messages in thread
From: Jan Kara @ 2021-08-11 12:02 UTC (permalink / raw)
  To: Amir Goldstein; +Cc: Jan Kara, Matthew Bobrowski, linux-fsdevel

On Tue 10-08-21 18:12:16, Amir Goldstein wrote:
> Hi Jan,
> 
> Following v2 addresses review comments from v1 [1]
> 
> [1] https://lore.kernel.org/linux-fsdevel/20210803180344.2398374-1-amir73il@gmail.com/

Thanks for the patches and for the review to Matthew. I've picked up the
patches to my tree.

								Honza

> 
> Changes since v1:
> - Rebase on 5.14-rc5 + pidfd patches
> - Added RVB
> - Helper to get connector's sb (Matthew)
> - Fix deadlock bug on umount (Jan)
> 
> Amir Goldstein (4):
>   fsnotify: replace igrab() with ihold() on attach connector
>   fsnotify: count s_fsnotify_inode_refs for attached connectors
>   fsnotify: count all objects with attached connectors
>   fsnotify: optimize the case of no marks of any type
> 
>  fs/notify/fsnotify.c     |  6 ++---
>  fs/notify/fsnotify.h     | 15 ++++++++++++
>  fs/notify/mark.c         | 52 ++++++++++++++++++++++++++++++----------
>  include/linux/fs.h       |  4 ++--
>  include/linux/fsnotify.h |  9 +++++++
>  5 files changed, 69 insertions(+), 17 deletions(-)
> 
> -- 
> 2.32.0
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 3/4] fsnotify: count all objects with attached connectors
  2021-08-11 11:37   ` Jan Kara
@ 2021-08-11 13:59     ` Amir Goldstein
  0 siblings, 0 replies; 9+ messages in thread
From: Amir Goldstein @ 2021-08-11 13:59 UTC (permalink / raw)
  To: Jan Kara; +Cc: Matthew Bobrowski, linux-fsdevel

On Wed, Aug 11, 2021 at 2:37 PM Jan Kara <jack@suse.cz> wrote:
>
> On Tue 10-08-21 18:12:19, Amir Goldstein wrote:
> > Rename s_fsnotify_inode_refs to s_fsnotify_connectors and count all
> > objects with attached connectors, not only inodes with attached
> > connectors.
> >
> > This will be used to optimize fsnotify() calls on sb without any
> > type of marks.
> >
> > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
>
> ... just a minor nit below ...
>
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 640574294216..d48d2018dfa4 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -1507,8 +1507,8 @@ struct super_block {
> >       /* Number of inodes with nlink == 0 but still referenced */
> >       atomic_long_t s_remove_count;
> >
> > -     /* Pending fsnotify inode refs */
> > -     atomic_long_t s_fsnotify_inode_refs;
> > +     /* Number of inode/mount/sb objects that are being watched */
> > +     atomic_long_t s_fsnotify_connectors;
>
> I've realized inode watches will be double-accounted because we increment
> s_fsnotify_connectors both when attaching a connector and when grabbing
> inode reference. It doesn't really matter and avoiding this would require
> special treatment of inode connectors (we need to decrement
> s_fsnotify_connectors only after dropping inode reference). So I'll just
> reflect this in the comment here so that we don't forget.

Great!
I actually have a patch to make the inode reference optional
per backend/mark [1], so keeping double account makes things
a lot easier for that code as well.

Thanks,
Amir.

[1] https://github.com/amir73il/linux/commits/fsnotify-volatile

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-08-11 14:00 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-08-10 15:12 [PATCH v2 0/4] Performance optimization for no fsnotify marks Amir Goldstein
2021-08-10 15:12 ` [PATCH v2 1/4] fsnotify: replace igrab() with ihold() on attach connector Amir Goldstein
2021-08-10 15:12 ` [PATCH v2 2/4] fsnotify: count s_fsnotify_inode_refs for attached connectors Amir Goldstein
2021-08-10 15:12 ` [PATCH v2 3/4] fsnotify: count all objects with " Amir Goldstein
2021-08-10 21:49   ` Matthew Bobrowski
2021-08-11 11:37   ` Jan Kara
2021-08-11 13:59     ` Amir Goldstein
2021-08-10 15:12 ` [PATCH v2 4/4] fsnotify: optimize the case of no marks of any type Amir Goldstein
2021-08-11 12:02 ` [PATCH v2 0/4] Performance optimization for no fsnotify marks Jan Kara

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).