Re: [PATCH v7 21/28] fanotify: Support merging of error events

From: Jan Kara <jack@suse.cz>
To: Gabriel Krisman Bertazi <krisman@collabora.com>
Cc: jack@suse.com, amir73il@gmail.com, djwong@kernel.org,
	tytso@mit.edu, dhowells@redhat.com, khazhy@google.com,
	linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
	linux-api@vger.kernel.org, repnop@google.com,
	kernel@collabora.com
Subject: Re: [PATCH v7 21/28] fanotify: Support merging of error events
Date: Fri, 15 Oct 2021 14:43:37 +0200	[thread overview]
Message-ID: <20211015124337.GK23102@quack2.suse.cz> (raw)
In-Reply-To: <20211014213646.1139469-22-krisman@collabora.com>

On Thu 14-10-21 18:36:39, Gabriel Krisman Bertazi wrote:
> Error events (FAN_FS_ERROR) against the same file system can be merged
> by simply iterating the error count.  The hash is taken from the fsid,
> without considering the FH.  This means that only the first error object
> is reported.
> 
> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>

...

> +static void fanotify_merge_error_event(struct fanotify_error_event *dest,
> +				       struct fanotify_error_event *origin)
> +{
> +	dest->err_count++;
> +}
> +
> +static void fanotify_merge_event(struct fanotify_event *dest,
> +				 struct fanotify_event *origin)
> +{
> +	dest->mask |= origin->mask;
> +
> +	if (origin->type == FANOTIFY_EVENT_TYPE_FS_ERROR)
> +		fanotify_merge_error_event(FANOTIFY_EE(dest),
> +					   FANOTIFY_EE(origin));
> +}
> +
>  /* Limit event merges to limit CPU overhead per event */
>  #define FANOTIFY_MAX_MERGE_EVENTS 128
>  
> @@ -175,7 +204,7 @@ static int fanotify_merge(struct fsnotify_group *group,
>  		if (++i > FANOTIFY_MAX_MERGE_EVENTS)
>  			break;
>  		if (fanotify_should_merge(old, new)) {
> -			old->mask |= new->mask;
> +			fanotify_merge_event(old, new);

I guess no need for two functions (fanotify_merge_event(),
fanotify_merge_error_event()) for three lines of code? I'd just put those
three lines into fanotify_merge().

> @@ -591,6 +621,9 @@ static struct fanotify_event *fanotify_alloc_error_event(
>  		return NULL;
>  
>  	fee->fae.type = FANOTIFY_EVENT_TYPE_FS_ERROR;
> +	fee->err_count = 1;
> +
> +	*hash ^= fanotify_hash_fsid(fsid);
>  
>  	return &fee->fae;
>  }

As Amir mentioned, you should set fsid here.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR