Re: [PATCH] fsnotify: avoid unrelated mark reaper waits during group teardown

[Jan Kara]

On Thu 10-09-26 01:08:33, Jia Zhu wrote:
> fsnotify_destroy_group() unconditionally flushes the global mark reaper,
> so an empty inotify instance can hang on unrelated reclamation during
> close or task exit:
> 
>   do_exit
>     __fput
>       inotify_release
>         fsnotify_destroy_group
>           fsnotify_wait_marks_destroyed
>             __flush_work
> 
> Skip the flush when only the closing reference remains, allowing teardown
> to finish without waiting for unrelated SRCU readers.

Yes, but is it really practically relevant? The optimization you add really
only works in the case when you have no notification marks in a group when
entering inotify_release(). Usually though you actually want to get
notified about something and thus do have some notification marks :) and in
that case fsnotify_clear_marks_by_group() -> fsnotify_put_mark() will add
marks to the list for the reaper to process. So please explain why this
corner case practically matters to you.

								Honza

> Verified in QEMU: empty groups and groups whose marks have been reclaimed
> exit while an unrelated SRCU reader remains held; groups with pending
> reclamation still wait.
> 
> Signed-off-by: Jia Zhu <zhujia.zj@xxxxxxxxxxxxx>
> ---
>  fs/notify/group.c | 26 +++++++++++++++-----------
>  1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/notify/group.c b/fs/notify/group.c
> index b56d1c1d9644a..052a419ee52a6 100644
> --- a/fs/notify/group.c
> +++ b/fs/notify/group.c
> @@ -46,6 +46,7 @@ void fsnotify_group_stop_queueing(struct fsnotify_group *group)
>   * the group reference.
>   * Note that another thread calling fsnotify_clear_marks_by_group() may still
>   * hold a ref to the group.
> + * The caller must hold a reference and exclude new marks.
>   */
>  void fsnotify_destroy_group(struct fsnotify_group *group)
>  {
> @@ -67,19 +68,22 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
>  	 */
>  	wait_event(group->notification_waitq, !atomic_read(&group->user_waits));
>  
> -	/*
> -	 * Wait until all marks get really destroyed. We could actually destroy
> -	 * them ourselves instead of waiting for worker to do it, however that
> -	 * would be racy as worker can already be processing some marks before
> -	 * we even entered fsnotify_destroy_group().
> -	 */
> -	fsnotify_wait_marks_destroyed();
> +	/* Even detached marks hold a group reference until final destruction. */
> +	if (refcount_read(&group->refcnt) == 1) {
> +		/*
> +		 * Pair the refcount read and this barrier with the release
> +		 * decrement in fsnotify_put_group() (refcount_dec_and_test()),
> +		 * ordering mark destruction before subsequent group teardown.
> +		 */
> +		smp_mb();
> +	} else {
> +		fsnotify_wait_marks_destroyed();
> +	}
>  
>  	/*
> -	 * Since we have waited for fsnotify_mark_srcu in
> -	 * fsnotify_mark_destroy_list() there can be no outstanding event
> -	 * notification against this group. So clearing the notification queue
> -	 * of all events is reliable now.
> +	 * Mark destruction waits for fsnotify_mark_srcu, so there can be no
> +	 * outstanding event notification against this group. Clearing the
> +	 * notification queue of all events is reliable now.
>  	 */
>  	fsnotify_flush_notify(group);
>  
> -- 
> 2.20.1
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR