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

[NeilBrown]

On Tue, 11 Aug 2026, NeilBrown wrote:
> On Tue, 11 Aug 2026, Miklos Szeredi wrote:
> > On Mon, 3 Aug 2026 at 03:39, NeilBrown <neilb@xxxxxxxxxxx> wrote:
> > 
> > > @@ -116,11 +117,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);
> > > +                       need_recalc = true;
> > >                 }
> > >         }
> > >
> > > +       if (need_recalc)
> > > +               need_recalc = dnotify_recalc_inode_mask(inode_mark);
> > >         spin_unlock(&inode_mark->lock);
> > > +       if (need_recalc)
> > > +               fsnotify_recalc_mask(inode_mark->connector);
> > 
> > Is the fsnotify_group_lock() held in this case?   I don't see it.
> 
> It isn't held.  Doesn't it need to be...
> It seems to protect marks, so maybe it does.
> 
> srcu seems to be used to protect this section, so maybe we can rely on
> that.

I dug into this some more, and we do rely on srcu, but don't need the
extra code below.
inode_mark->lock doesn't protect inode_mark->connector, so moving the
dereference out of the lock has no effect.
srcu_read_lock is taken before we get the ref to the mark, so the mark
and the connector cannot disappear underneath us.
A race could result in inode_mark->connector reading as NULL, but
fsnotify_recalc_mask() checks for NULL, so there is no risk for harm.

Thanks for encouraging me to dig into this.

NeilBrown

> Maybe:
> 
> diff --git a/fs/notify/dnotify/dnotify.c b/fs/notify/dnotify/dnotify.c
> index 7553fe0d7850..ba746284bded 100644
> --- a/fs/notify/dnotify/dnotify.c
> +++ b/fs/notify/dnotify/dnotify.c
> @@ -17,6 +17,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
>  #include <linux/fsnotify_backend.h>
> +#include "../fsnotify.h"
>  
>  static int dir_notify_enable __read_mostly = 1;
>  #ifdef CONFIG_SYSCTL
> @@ -94,6 +95,7 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>  	struct dnotify_struct *dn;
>  	struct dnotify_struct **prev;
>  	struct fown_struct *fown;
> +	struct fsnotify_mark_connector *conn;
>  	bool need_recalc = false;
>  	__u32 test_mask = mask & ~FS_EVENT_ON_CHILD;
>  
> @@ -123,9 +125,10 @@ static int dnotify_handle_event(struct fsnotify_mark *inode_mark, u32 mask,
>  
>  	if (need_recalc)
>  		need_recalc = dnotify_recalc_inode_mask(inode_mark);
> +	conn = srcu_dereference(inode_mark->connector, &fsnotify_mark_srcu);
>  	spin_unlock(&inode_mark->lock);
>  	if (need_recalc)
> -		fsnotify_recalc_mask(inode_mark->connector);
> +		fsnotify_recalc_mask(conn);
>  
>  	return 0;
>  }
> 
> Does that make sense?
> 
> Thanks,
> NeilBrown
> 
>