On Tue 23-06-15 12:05:51, Ashish Sangwan wrote:
> > Looking into this in more detail, it might be worthwhile to revisit how
> > mark_mutex is used since at least fanotify and dnotify use it for more than
> > just a protection of list of group marks and untangling this would simplify
> > things. But that's a longer term goal.
> >
> > A relatively simple fix for your issue is to split group list of marks into
> > a list of inode marks and a list of mount marks. Then destroying becomes
> > much simpler because we always discard the whole list (or both of them) and
> > we can easily avoid problems with list corruption when dropping the
> > mark_mutex. I can write the patch later or you can do that if you are
> Sorry I could not understand why the group's list of marks needs to be split.
> I was browsing through the old code, from the days mark_mutex was not present
> and it looked like below:
> void fsnotify_clear_marks_by_group_flags(struct fsnotify_group *group,
> unsigned int flags)
> {
> struct fsnotify_mark *lmark, *mark;
> LIST_HEAD(free_list);
>
> spin_lock(&group->mark_lock);
> list_for_each_entry_safe(mark, lmark, &group->marks_list, g_list) {
> if (mark->flags & flags) {
> list_add(&mark->free_g_list, &free_list);
> list_del_init(&mark->g_list);
> fsnotify_get_mark(mark);
> }
> }
> spin_unlock(&group->mark_lock);
>
> list_for_each_entry_safe(mark, lmark, &free_list, free_g_list) {
> fsnotify_destroy_mark(mark);
> fsnotify_put_mark(mark);
> }
> }
> How about using a temporary onstack list_head like above?
So we can use a temporary list_head for entries selected for destruction as
well. *But* in either case you have to be careful because even the
temporary free_list can be modified by concurrent mark destruction e.g. from
fsnotify_free_marks_by_inode() call as each mark is in two lists - one
hanging from inode / mount and one hanging from the notification group.
Actually, looking into it even fsnotify_destroy_marks() doesn't seem to be
properly protected from the race. Sigh.
Honza
> > > Signed-off-by: Ashish Sangwan <a.sangwan@xxxxxxxxxxx>
> > > Reviewed-by: Amit Sahrawat <a.sahrawat@xxxxxxxxxxx>
> > > ---
> > > fs/notify/mark.c | 4 ----
> > > 1 files changed, 0 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> > > index 92e48c7..4ee419f 100755
> > > --- a/fs/notify/mark.c
> > > +++ b/fs/notify/mark.c
> > > @@ -157,8 +157,6 @@ void fsnotify_destroy_mark_locked(struct fsnotify_mark *mark,
> > >
> > > if (inode && (mark->flags & FSNOTIFY_MARK_FLAG_OBJECT_PINNED))
> > > iput(inode);
> > > - /* release lock temporarily */
> > > - mutex_unlock(&group->mark_mutex);
> > >
> > > spin_lock(&destroy_lock);
> > > list_add(&mark->g_list, &destroy_list);
> > > @@ -191,8 +189,6 @@ void fsnotify_destroy_mark_locked(struct fsnotify_mark *mark,
> > > */
> > >
> > > atomic_dec(&group->num_marks);
> > > -
> > > - mutex_lock_nested(&group->mark_mutex, SINGLE_DEPTH_NESTING);
> > > }
> > >
> > > void fsnotify_destroy_mark(struct fsnotify_mark *mark,
> > > --
> > > 1.7.7
> > >
> > > --
> > Jan Kara <jack@xxxxxxx>
> > SUSE Labs, CR
--
Jan Kara <jack@xxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
