Re: [PATCH 1/7] fsnotify: generalize handle_inode_event()

[Jan Kara <jack@suse.cz>]

On Wed 02-12-20 14:07:07, Amir Goldstein wrote:
> The handle_inode_event() interface was added as (quoting comment):
> "a simple variant of handle_event() for groups that only have inode
> marks and don't have ignore mask".
> 
> In other words, all backends except fanotify.  The inotify backend
> also falls under this category, but because it required extra arguments
> it was left out of the initial pass of backends conversion to the
> simple interface.
> 
> This results in code duplication between the generic helper
> fsnotify_handle_event() and the inotify_handle_event() callback
> which also happen to be buggy code.
> 
> Generalize the handle_inode_event() arguments and add the check for
> FS_EXCL_UNLINK flag to the generic helper, so inotify backend could
> be converted to use the simple interface.
> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>

The patch looks good to me. Just one curious question below.

> +static int fsnotify_handle_inode_event(struct fsnotify_group *group,
> +				       struct fsnotify_mark *inode_mark,
> +				       u32 mask, const void *data, int data_type,
> +				       struct inode *dir, const struct qstr *name,
> +				       u32 cookie)
> +{
> +	const struct path *path = fsnotify_data_path(data, data_type);
> +	struct inode *inode = fsnotify_data_inode(data, data_type);
> +	const struct fsnotify_ops *ops = group->ops;
> +
> +	if (WARN_ON_ONCE(!ops->handle_inode_event))
> +		return 0;
> +
> +	if ((inode_mark->mask & FS_EXCL_UNLINK) &&
> +	    path && d_unlinked(path->dentry))
> +		return 0;

When I was looking at this condition I was wondering why do we check
d_unlinked() and not inode->i_nlink? When is there a difference?

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