On Thu 05-04-18 16:18:09, Amir Goldstein wrote:
> Make some code that handles marks of object types inode and vfsmount
> generic, so it can handle other object types.
>
> Introduce foreach_obj_type macros to iterate marks by object type
> with or without consulting a type mask.
>
> This is going to be used for adding mark of another object type
> (super block mark).
>
> Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> ---
> fs/notify/fsnotify.c | 54 ++++++++++++++++++++++++----------------
> fs/notify/mark.c | 23 +++++++++++------
> include/linux/fsnotify_backend.h | 38 +++++++++++++++++++++-------
> 3 files changed, 76 insertions(+), 39 deletions(-)
>
> diff --git a/fs/notify/fsnotify.c b/fs/notify/fsnotify.c
> index 665f681dd913..1759121db50d 100644
> --- a/fs/notify/fsnotify.c
> +++ b/fs/notify/fsnotify.c
> @@ -263,21 +263,31 @@ static struct fsnotify_mark *fsnotify_next_mark(struct fsnotify_mark *mark)
> /*
> * iter_info is a multi head priority queue of marks.
> * fsnotify_iter_first() picks a subset of marks from queue heads, all with the
> - * same priority and clears the type_mask for other marks.
> + * same group and clears the type_mask for other marks.
> * Returns the type_mask of the chosen subset.
> */
> static unsigned int fsnotify_iter_first(struct fsnotify_iter_info *iter_info)
> {
> - struct fsnotify_mark *inode_mark = iter_info->inode_mark;
> - struct fsnotify_mark *vfsmount_mark = iter_info->vfsmount_mark;
> -
> - if (inode_mark && vfsmount_mark) {
> - int cmp = fsnotify_compare_groups(inode_mark->group,
> - vfsmount_mark->group);
> - if (cmp > 0)
> - iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_INODE_FL;
> - else if (cmp < 0)
> - iter_info->type_mask &= ~FSNOTIFY_OBJ_TYPE_VFSMOUNT_FL;
> + struct fsnotify_group *max_prio_group = NULL;
> + unsigned int type_mask = iter_info->type_mask;
> + struct fsnotify_mark *mark;
> + int type;
> +
> + if (!type_mask || is_power_of_2(type_mask))
> + return type_mask;
> +
> + /* Choose max prio group among groups of all queue heads */
> + fsnotify_iter_foreach_obj_type_mark(iter_info, type, mark) {
Does this macro really bring any benefit over:
fsnotify_foreach_obj_type(type) {
mark = iter_info->marks[type];
Frankly the second makes it clearer what's going on and you don't have to
wonder whether unset types are also included or not... What might be worth it
is something like fsnotify_iter_foreach_mark() which would iterate over all
*set* marks. But then there's the question whether you are iterating over
all types set in the mask or all types set in the ->marks array so probably
that will be error prone.
> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
> index ef44808b28ca..3df2d5ecf0b7 100644
> --- a/fs/notify/mark.c
> +++ b/fs/notify/mark.c
> @@ -294,12 +294,12 @@ static void fsnotify_put_mark_wake(struct fsnotify_mark *mark)
>
> bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
> {
> - /* This can fail if mark is being removed */
> - if (!fsnotify_get_mark_safe(iter_info->inode_mark))
> - return false;
> - if (!fsnotify_get_mark_safe(iter_info->vfsmount_mark)) {
> - fsnotify_put_mark_wake(iter_info->inode_mark);
> - return false;
> + int type;
> +
> + fsnotify_foreach_obj_type(type) {
> + /* This can fail if mark is being removed */
> + if (!fsnotify_get_mark_safe(iter_info->marks[type]))
> + goto fail;
> }
>
> /*
> @@ -310,13 +310,20 @@ bool fsnotify_prepare_user_wait(struct fsnotify_iter_info *iter_info)
> srcu_read_unlock(&fsnotify_mark_srcu, iter_info->srcu_idx);
>
> return true;
> +
> +fail:
> + while (type-- > 0)
Can you please rewrite it as:
for (type--; type >= 0; type--)
I know I'm lame but your version made me think for a while whether index 0
and the last index will be actually treated correctly. And we had bugs like
this in the past...
> + fsnotify_put_mark_wake(iter_info->marks[type]);
> + return false;
> }
>
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
