Re: [PATCH 1/2] fsnotify: introduce mark type iterator

From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>,
	Matthew Bobrowski <mbobrowski@mbobrowski.org>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH 1/2] fsnotify: introduce mark type iterator
Date: Wed, 11 May 2022 14:54:40 +0200	[thread overview]
Message-ID: <20220511125440.5zsuzn7eemdvfksi@quack3.lan> (raw)
In-Reply-To: <20220511092914.731897-2-amir73il@gmail.com>

On Wed 11-05-22 12:29:13, Amir Goldstein wrote:
> fsnotify_foreach_iter_mark_type() is used to reduce boilerplate code
> of iteratating all marks of a specific group interested in an event
> by consulting the iterator report_mask.
> 
> Use an open coded version of that iterator in fsnotify_iter_next()
> that collects all marks of the current iteration group without
> consulting the iterator report_mask.
> 
> At the moment, the two iterator variants are the same, but this
> decoupling will allow us to exclude some of the group's marks from
> reporting the event, for example for event on child and inode marks
> on parent did not request to watch events on children.
> 
> Fixes: 2f02fd3fa13e ("fanotify: fix ignore mask logic for events on child and on dir")
> Reported-by: Jan Kara <jack@suse.com>
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

Mostly looks good. Two nits below.

>  /*
> - * Pop from iter_info multi head queue, the marks that were iterated in the
> + * Pop from iter_info multi head queue, the marks that belong to the group of
>   * current iteration step.
>   */
>  static void fsnotify_iter_next(struct fsnotify_iter_info *iter_info)
>  {
> +	struct fsnotify_mark *mark;
>  	int type;
>  
>  	fsnotify_foreach_iter_type(type) {
> -		if (fsnotify_iter_should_report_type(iter_info, type))
> +		mark = iter_info->marks[type];
> +		if (mark && mark->group == iter_info->current_group)
>  			iter_info->marks[type] =
>  				fsnotify_next_mark(iter_info->marks[type]);

Wouldn't it be more natural here to use the new helper
fsnotify_foreach_iter_mark_type()? In principle we want to advance mark
types which were already reported...

> @@ -438,6 +438,9 @@ FSNOTIFY_ITER_FUNCS(sb, SB)
>  
>  #define fsnotify_foreach_iter_type(type) \
>  	for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++)
> +#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
> +	for (type = 0; type < FSNOTIFY_ITER_TYPE_COUNT; type++) \
> +		if (!(mark = fsnotify_iter_mark(iter, type))) {} else

Hum, you're really inventive here ;) I'd rather go for something a bit more
conservative and readable like:

static inline int fsnotify_iter_step(struct fsnotify_iter_info *iter, int type,
				     struct fsnotify_mark **markp)
{
	while (type < FSNOTIFY_ITER_TYPE_COUNT) {
		*markp = fsnotify_iter_mark(iter, type);
		if (*markp)
			break;
		type++;
	}
	return type;
}

#define fsnotify_foreach_iter_mark_type(iter, mark, type) \
	for (type = 0; \
	     (type = fsnotify_iter_step(iter, type, &mark)) < FSNOTIFY_ITER_TYPE_COUNT; \
	     type++)

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