diff mbox series

[v4,3/5] dnotify: move fsnotify_recalc_mask() outside spinlock

Message ID 20221111220614.991928-4-stephen.s.brennan@oracle.com
State New
Headers
Series fsnotify: fix softlockups iterating over d_subdirs |

Commit Message

Stephen Brennan Nov. 11, 2022, 10:06 p.m. UTC 
  In order to allow sleeping during fsnotify_recalc_mask(), we need to
ensure no callers are holding a spinlock.

Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>
---
 fs/notify/dnotify/dnotify.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

Comments

Amir Goldstein Nov. 12, 2022, 9:06 a.m. UTC | #1 
On Sat, Nov 12, 2022 at 12:06 AM Stephen Brennan
<stephen.s.brennan@oracle.com> wrote:
>
> In order to allow sleeping during fsnotify_recalc_mask(), we need to
> ensure no callers are holding a spinlock.
>
> Signed-off-by: Stephen Brennan <stephen.s.brennan@oracle.com>

Reviewed-by: Amir Goldstein <amir73il@gmail.com>

small suggestion below.

> ---
>  fs/notify/dnotify/dnotify.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 190aa717fa32..a9f05b3cf5ea 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -58,10 +58,10 @@ struct dnotify_mark {
>   * dnotify cares about for that inode may change.  This function runs the
>   * list of everything receiving dnotify events about this directory and calculates
>   * the set of all those events.  After it updates what dnotify is interested in
> - * it calls the fsnotify function so it can update the set of all events relevant
> - * to this inode.
> + * it returns true if fsnotify_recalc_mask() should be called to update the set
> + * of all events related to this inode.
>   */
> -static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
> +static bool dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
>  {
>         __u32 new_mask = 0;
>         struct dnotify_struct *dn;
> @@ -74,10 +74,9 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
>         for (dn = dn_mark->dn; dn != NULL; dn = dn->dn_next)
>                 new_mask |= (dn->dn_mask & ~FS_DN_MULTISHOT);
>         if (fsn_mark->mask == new_mask)
> -               return;
> +               return false;
>         fsn_mark->mask = new_mask;
> -
> -       fsnotify_recalc_mask(fsn_mark->connector);
> +       return true;
>  }
>
>  /*
> @@ -97,6 +96,7 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>         struct dnotify_struct **prev;
>         struct fown_struct *fown;
>         __u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
> +       bool recalc = false;
>
>         /* not a dir, dnotify doesn't care */
>         if (!dir && !(mask & FS_ISDIR))
> @@ -118,12 +118,15 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>                 else {
>                         *prev = dn->dn_next;
>                         kmem_cache_free(dnotify_struct_cache, dn);
> -                       dnotify_recalc_inode_mask(inode_mark);
> +                       recalc = dnotify_recalc_inode_mask(inode_mark);
>                 }
>         }
>
>         spin_unlock(&inode_mark->lock);
>
> +       if (recalc)
> +               fsnotify_recalc_mask(inode_mark->connector);
> +
>         return 0;
>  }
>
> @@ -158,6 +161,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
>         struct dnotify_struct **prev;
>         struct inode *inode;
>         bool free = false;
> +       bool recalc = false;
>
>         inode = file_inode(filp);
>         if (!S_ISDIR(inode->i_mode))
> @@ -176,7 +180,7 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
>                 if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {
>                         *prev = dn->dn_next;
>                         kmem_cache_free(dnotify_struct_cache, dn);
> -                       dnotify_recalc_inode_mask(fsn_mark);
> +                       recalc = dnotify_recalc_inode_mask(fsn_mark);
>                         break;
>                 }
>                 prev = &dn->dn_next;
> @@ -184,6 +188,9 @@ void dnotify_flush(struct file *filp, fl_owner_t id)
>
>         spin_unlock(&fsn_mark->lock);
>
> +       if (recalc)
> +               fsnotify_recalc_mask(fsn_mark->connector);
> +
>         /* nothing else could have found us thanks to the dnotify_groups
>            mark_mutex */
>         if (dn_mark->dn == NULL) {
> @@ -268,6 +275,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>         struct file *f;
>         int destroy = 0, error = 0;
>         __u32 mask;
> +       bool recalc = false;
>
>         /* we use these to tell if we need to kfree */
>         new_fsn_mark = NULL;
> @@ -377,9 +385,11 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
>         else if (error == -EEXIST)
>                 error = 0;
>
> -       dnotify_recalc_inode_mask(fsn_mark);
> +       recalc = dnotify_recalc_inode_mask(fsn_mark);
>  out:
>         spin_unlock(&fsn_mark->lock);
> +       if (recalc)
> +               fsnotify_recalc_mask(fsn_mark->connector);
>
>         if (destroy)
>                 fsnotify_detach_mark(fsn_mark);

I'd do else if (recalc)

just to emphasise that destroy and recalc are mutually exclusive.

Thanks,
Amir.
diff mbox series

Patch

diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 190aa717fa32..a9f05b3cf5ea 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -58,10 +58,10 @@  struct dnotify_mark {
  * dnotify cares about for that inode may change.  This function runs the
  * list of everything receiving dnotify events about this directory and calculates
  * the set of all those events.  After it updates what dnotify is interested in
- * it calls the fsnotify function so it can update the set of all events relevant
- * to this inode.
+ * it returns true if fsnotify_recalc_mask() should be called to update the set
+ * of all events related to this inode.
  */
-static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
+static bool dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
 {
 	__u32 new_mask = 0;
 	struct dnotify_struct *dn;
@@ -74,10 +74,9 @@  static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
 	for (dn = dn_mark->dn; dn != NULL; dn = dn->dn_next)
 		new_mask |= (dn->dn_mask & ~FS_DN_MULTISHOT);
 	if (fsn_mark->mask == new_mask)
-		return;
+		return false;
 	fsn_mark->mask = new_mask;
-
-	fsnotify_recalc_mask(fsn_mark->connector);
+	return true;
 }
 
 /*
@@ -97,6 +96,7 @@  static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 	struct dnotify_struct **prev;
 	struct fown_struct *fown;
 	__u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
+	bool recalc = false;
 
 	/* not a dir, dnotify doesn't care */
 	if (!dir && !(mask & FS_ISDIR))
@@ -118,12 +118,15 @@  static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
 		else {
 			*prev = dn->dn_next;
 			kmem_cache_free(dnotify_struct_cache, dn);
-			dnotify_recalc_inode_mask(inode_mark);
+			recalc = dnotify_recalc_inode_mask(inode_mark);
 		}
 	}
 
 	spin_unlock(&inode_mark->lock);
 
+	if (recalc)
+		fsnotify_recalc_mask(inode_mark->connector);
+
 	return 0;
 }
 
@@ -158,6 +161,7 @@  void dnotify_flush(struct file *filp, fl_owner_t id)
 	struct dnotify_struct **prev;
 	struct inode *inode;
 	bool free = false;
+	bool recalc = false;
 
 	inode = file_inode(filp);
 	if (!S_ISDIR(inode->i_mode))
@@ -176,7 +180,7 @@  void dnotify_flush(struct file *filp, fl_owner_t id)
 		if ((dn->dn_owner == id) && (dn->dn_filp == filp)) {
 			*prev = dn->dn_next;
 			kmem_cache_free(dnotify_struct_cache, dn);
-			dnotify_recalc_inode_mask(fsn_mark);
+			recalc = dnotify_recalc_inode_mask(fsn_mark);
 			break;
 		}
 		prev = &dn->dn_next;
@@ -184,6 +188,9 @@  void dnotify_flush(struct file *filp, fl_owner_t id)
 
 	spin_unlock(&fsn_mark->lock);
 
+	if (recalc)
+		fsnotify_recalc_mask(fsn_mark->connector);
+
 	/* nothing else could have found us thanks to the dnotify_groups
 	   mark_mutex */
 	if (dn_mark->dn == NULL) {
@@ -268,6 +275,7 @@  int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
 	struct file *f;
 	int destroy = 0, error = 0;
 	__u32 mask;
+	bool recalc = false;
 
 	/* we use these to tell if we need to kfree */
 	new_fsn_mark = NULL;
@@ -377,9 +385,11 @@  int fcntl_dirnotify(int fd, struct file *filp, unsigned long arg)
 	else if (error == -EEXIST)
 		error = 0;
 
-	dnotify_recalc_inode_mask(fsn_mark);
+	recalc = dnotify_recalc_inode_mask(fsn_mark);
 out:
 	spin_unlock(&fsn_mark->lock);
+	if (recalc)
+		fsnotify_recalc_mask(fsn_mark->connector);
 
 	if (destroy)
 		fsnotify_detach_mark(fsn_mark);