Re: [PATCH v1 03/12] fsnotify: don't hold a spin_lock across fsnotify_recalc_mask() calls.

On Wed 19-08-26 11:09:08, Amir Goldstein wrote:
> On Wed, Aug 19, 2026 at 1:30 AM NeilBrown <[email protected]> wrote:
> > On Mon, 17 Aug 2026, Amir Goldstein wrote:
> > > Maybe this is the way out.
> > >
> > > dnotify_recalc_inode_mask() can only remove bits from i_fsnotify_mask,
> > > so it is an optimization.
> > >
> > > If we just remove fsnotify_recalc_mask() call from
> > > dnotify_recalc_inode_mask(), then i_fsnotify_mask will be updated when
> > > dnotify_mask gets removed eventually.
> >
> > I had thought something along these lines too.  I don't think we need to
> > drop all of fsnotify_recalc_mask(), only the
> > fsnotify_conn_set_children_dentry_flags() part that walks the d_children
> > list.  And that only happens when we add to the mask, not when bits are
> > cleared.  So maybe the fsnotify_conn_set_children_dentry_flags() simply
> > never happens in this context so it doesn't need fixing.

I agree with the "fsnotify_conn_set_children_dentry_flags() never happens
from this path" part. I'm not sure I follow the "it doesn't need fixing"
part - do you mean you will just not touch where dnotify calls
fsnotify_recalc_mask()? In that case I agree.

> > I would prefer to make that clear from the code with a patch like the
> > following.

Definitely.

> > Note that I'm not (yet) convinced by the locking argument, but maybe we
> > don't need to pursue it.

Not sure what you mean here...

> > diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> > index 9fb73bafd41d..be66d4142563 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_inatomic(fsn_mark->connector);
> 
> I don't think skipping update_children is correct when called
> from fcntl_dirnotify().

Yes, it is not. Dnotify places mark on directory and sets FS_EVENT_ON_CHILD
so it depends on dentry flags to be updated from fcntl_dirnotify().

> From 73bbfcd1b21a0fc723999aee36af662f36ba4dd2 Mon Sep 17 00:00:00 2001
> From: Amir Goldstein <[email protected]>
> Date: Wed, 19 Aug 2026 10:56:40 +0200
> Subject: [PATCH] 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.
> 
> Fixes: 172e422ffea20 ("fsnotify: clear PARENT_WATCHED flags lazily")
> Signed-off-by: Amir Goldstein <[email protected]>

Looks good to me, Neil will you pick this into your series or should I pick
the patch into my tree rightaway?

> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index b2640d836a712..731e430f2f3c6 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -317,20 +317,21 @@ 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.
>   */
> -void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn)
> +void fsnotify_recalc_mask(struct fsnotify_mark_connector *conn,
> +			  bool update_children)
>  {
> -	bool update_children;

As a side note once Neil allows the dentry iteration to sleep, we should
document in the comment above that mark->lock can be used only for
!update_children case and perhaps assert it in the function as well.

								Honza
-- 
Jan Kara <[email protected]>
SUSE Labs, CR