Re: [PATCH v1 03/12] fsnotify: don't hold a spin_lock across fsnotify_recalc_mask() calls.
From: Amir Goldstein
Date: Wed Aug 26 2026 - 07:45:47 EST
On Sun, Aug 23, 2026 at 3:21 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
>
> On Wed, 19 Aug 2026, Amir Goldstein wrote:
> > On Wed, Aug 19, 2026 at 1:30 AM NeilBrown <neilb@xxxxxxxxxxx> wrote:
> > > --- a/fs/notify/dnotify/dnotify.c
> > > +++ b/fs/notify/dnotify/dnotify.c
> > > @@ -75,7 +75,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
> > > return;
> > > fsn_mark->mask = new_mask;
> > >
> > > - fsnotify_recalc_mask(fsn_mark->connector);
> > > + fsnotify_recalc_mask_inatomic(fsn_mark->connector);
> >
> > I don't think skipping update_children is correct when called
> > from fcntl_dirnotify().
>
> Thanks for catching that.
> I agree we shouldn't skip here, but also we mustn't call
> fsnotify_conn_set_children_dentry_flags() at this point because
> a spinlock is held.
> In fcntl_dirnotify() is it safe to delay that call until after the
> lock is dropped because fsnotify_group_lock() is still held.
>
> So I would like to keep that part of my patch - the part where
> fsnotify_recalc_mask() (and dnotify_recalc_inode_mask()) returns the
> "update_children" flag and the caller is responsible for calling
> fsnotify_conn_set_children_dentry_flags().
It's just a bit over complicated for my taste.
We do not need to optimize the dnotify case this much.
See my v2 patch, which addresses your concern and keeps
our internal API simpler (to my taste).
There is nothing technically wrong with your patch.
It's just my preference is different and Jan seems to be on the same page.
>
> Some callers call fsnotify_conn_set_children_dentry_flags() immediately,
> some wait until a spinlock is dropped, dnotify_handle_event() wouldn't
> call it at all.
There are 3 categories and my patch handles them by explicitly
requesting update_children or explicitly calling the children walk
outside of spinlock in the single special case.
>
> Your patch might be a good addition but it doesn't block the d_children
> walk in all cases where a spinlock is held, so it doesn't help my
> immediate need.
v2 should be ok?
Thanks,
Amir.
From b85c6c529b6ff92dd3d1c53c8de95c9bb4b13e8d Mon Sep 17 00:00:00 2001
From: Amir Goldstein <amir73il@xxxxxxxxx>
Date: Wed, 19 Aug 2026 10:56:40 +0200
Subject: [PATCH v2] fsnotify: update children flags only when adding mask bits
Since we only ever set the PARENT_WATCHED flag from
fsnotify_recalc_mask(), there is never a need to update children flags
when removing mark mask bits only when adding them.
Also avoid update children with mark spinlock held.
This was only done by dnotify and can be easily avoided.
Fixes: 172e422ffea20 ("fsnotify: clear PARENT_WATCHED flags lazily")
Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
---
fs/notify/dnotify/dnotify.c | 3 ++-
fs/notify/fanotify/fanotify_user.c | 4 ++--
fs/notify/inotify/inotify_user.c | 2 +-
fs/notify/mark.c | 22 ++++++++++++----------
include/linux/fsnotify_backend.h | 10 ++++++++--
5 files changed, 25 insertions(+), 16 deletions(-)
diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
index 9fb73bafd41d2..076ba4123280c 100644
--- a/fs/notify/dnotify/dnotify.c
+++ b/fs/notify/dnotify/dnotify.c
@@ -75,7 +75,7 @@ static void dnotify_recalc_inode_mask(struct fsnotify_mark *fsn_mark)
return;
fsn_mark->mask = new_mask;
- fsnotify_recalc_mask(fsn_mark->connector);
+ fsnotify_recalc_mask(fsn_mark->connector, false);
}
/*
@@ -380,6 +380,7 @@ int fcntl_dirnotify(int fd, struct file *filp, unsigned int arg)
dnotify_recalc_inode_mask(fsn_mark);
out:
spin_unlock(&fsn_mark->lock);
+ fsnotify_conn_set_children_dentry_flags(fsn_mark->connector);
if (destroy)
fsnotify_detach_mark(fsn_mark);
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index a32c6634d5927..700e353c425a6 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -1264,7 +1264,7 @@ static int fanotify_remove_mark(struct fsnotify_group *group,
removed = fanotify_mark_remove_from_mask(fsn_mark, mask, flags,
umask, &destroy_mark);
if (removed & fsnotify_conn_mask(fsn_mark->connector))
- fsnotify_recalc_mask(fsn_mark->connector);
+ fsnotify_recalc_mask(fsn_mark->connector, false);
if (destroy_mark)
fsnotify_detach_mark(fsn_mark);
fsnotify_group_unlock(group);
@@ -1538,7 +1538,7 @@ static int fanotify_add_mark(struct fsnotify_group *group,
recalc = fanotify_mark_add_to_mask(fsn_mark, mask, fan_flags);
if (recalc)
- fsnotify_recalc_mask(fsn_mark->connector);
+ fsnotify_recalc_mask(fsn_mark->connector, true);
out:
fsnotify_group_unlock(group);
diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5f19c24ec187f..6d84c5116749d 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -562,7 +562,7 @@ static int inotify_update_existing_watch(struct fsnotify_group *group,
fsn_mark->flags |= inotify_arg_to_flags(arg);
spin_unlock(&fsn_mark->lock);
- fsnotify_recalc_mask(fsn_mark->connector);
+ fsnotify_recalc_mask(fsn_mark->connector, true);
/* return the wd */
ret = i_mark->wd;
diff --git a/fs/notify/mark.c b/fs/notify/mark.c
index b2640d836a712..564e8f9318a17 100644
--- a/fs/notify/mark.c
+++ b/fs/notify/mark.c
@@ -307,7 +307,7 @@ static bool fsnotify_conn_watches_children(
return fsnotify_inode_watches_children(fsnotify_conn_inode(conn));
}
-static void fsnotify_conn_set_children_dentry_flags(
+void fsnotify_conn_set_children_dentry_flags(
struct fsnotify_mark_connector *conn)
{
if (conn->type != FSNOTIFY_OBJ_TYPE_INODE)
@@ -317,20 +317,22 @@ static void fsnotify_conn_set_children_dentry_flags(
}
/*
- * Calculate mask of events for a list of marks. The caller must make sure
- * connector and connector->obj cannot disappear under us. Callers achieve
- * this by holding a mark->lock or mark->group->mark_mutex for a mark on this
- * list.
+ * Calculate mask of events for a list of marks.
+ * If @update_children is true, update children dentry flags if needed.
+ * The caller must make sure connector and connector->obj cannot disappear under
+ * us. Callers achieve this by holding a mark->lock or mark->group->mark_mutex
+ * for a mark on this list.
+ * When @update_children is true, mark->lock must not be held by the caller.
*/
-void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
+void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn,
+ bool update_children)
{
- bool update_children;
if (!conn)
return;
spin_lock(&conn->lock);
- update_children = !fsnotify_conn_watches_children(conn);
+ update_children &= !fsnotify_conn_watches_children(conn);
fsnotify_recalc_mask_set_iref(conn);
update_children &= fsnotify_conn_watches_children(conn);
spin_unlock(&conn->lock);
@@ -368,7 +370,7 @@ void fsnotify_modify_mark_mask(struct fsnotify_mark *mark, u32 set, u32 clear)
spin_unlock(&mark->lock);
if (recalc)
- fsnotify_recalc_mask(mark->connector);
+ fsnotify_recalc_mask(mark->connector, !!set);
}
EXPORT_SYMBOL_GPL(fsnotify_modify_mark_mask);
@@ -998,7 +1000,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark,
if (ret)
goto err;
- fsnotify_recalc_mask(mark->connector);
+ fsnotify_recalc_mask(mark->connector, true);
return ret;
err:
diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
index 618eed4d6d724..e90ceeebceb5d 100644
--- a/include/linux/fsnotify_backend.h
+++ b/include/linux/fsnotify_backend.h
@@ -885,8 +885,14 @@ static inline __u32 fsnotify_calc_mask(struct fsnotify_mark *mark)
/* Get mask of events for a list of marks */
extern __u32 fsnotify_conn_mask(struct fsnotify_mark_connector *conn);
-/* Calculate mask of events for a list of marks */
-extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn);
+/*
+ * Calculate mask of events for a list of marks and update children dentry
+ * flag is needed.
+ */
+extern void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn,
+ bool update_children);
+extern void fsnotify_conn_set_children_dentry_flags(
+ 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 */
--
2.55.0