On Tue, Apr 17, 2018 at 8:01 PM, Jan Kara <jack@xxxxxxx> wrote:
> 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];
You mean:
mark = fsnotify_iter_type_mark(info, type, mark);
Which is non NULL only if type was selected.
Needs a better name??
How about fsnotify_iter_test_type_mark() to counter
fsnotify_iter_set_type_mark()?
>
> 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.
I had such a macro in an earlier version, but it was quite ugly so I let it go.
I'll get rid of fsnotify_iter_foreach_obj_type_mark() and see if it looks nicer.
>
>> 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...
>
Sure.
Thanks,
Amir.
