On Wed, Apr 18, 2018 at 11:34 AM, Jan Kara <jack@xxxxxxx> wrote:
> On Tue 17-04-18 22:11:04, Amir Goldstein wrote:
>> 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.
>
> Hum, probably but it seems a bit confusing (and thus error prone in the
> long term). So how about making fsnotify_iter_next() just advancing mark
> pointers based on report_mask (to move over marks that were already
> reported) and then clearing report_mask.
OK, but no need to clear report_mask in fsnotify_iter_next(), because
select_report_types will recalculate the report mask.
> fsnotify_iter_select_report_types() would the walk over iter_info->marks
> array, consider all non-NULL entries for reporting, select among them
> those with the highest priority, and set report_mask appropriately.
> That way we won't even need fsnotify_iter_set_{inode|vfsmount}_mark()
> macros.
Right.
>
>> Needs a better name??
>> How about fsnotify_iter_test_type_mark() to counter
>> fsnotify_iter_set_type_mark()?
>
> If there are not many places, that need fsnotify_iter_type_mark() (which
> currently it seems there are not), then I'd just make it
> fsnotify_iter_should_report_type(iter, type) which returns bool - whether
> that type should be reported to or not. And then just explicitely use
> iter->marks[type] if type should be reported. IMO fsnotify_iter_type_mark()
> does not offer much advantage over this as you have to test for mark being
> NULL anyway.
>
I left convenience macros:
fsnotify_iter_should_report_type(iter, type)
fsnotify_iter_set_report_type(iter, type)
fsnotify_iter_set_report_type_mark(iter, type, mark)
> If there are many places that need fsnotify_iter_type_mark(), then I guess
> we could provide iteration only over types that should be reported. Does
> that sound good?
>
Yes, result looks much better IMO.
Rabased on your for_test branch and push to
https://github.com/amir73il/linux/commits/fanotify_sb
Thanks!
Amir.
