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.
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.
> 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.
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?
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
