| Message ID | 20220329074904.2980320-13-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Evictable fanotify marks | expand |
On Tue 29-03-22 10:49:00, Amir Goldstein wrote: > Handle FAN_MARK_IGNORED_SURV_MODIFY flag change in a helper that > is called after updating the mark mask. > > Move recalc of object mask inside fanotify_mark_add_to_mask() which > makes the code a bit simpler to follow. > > Add also helper to translate fsnotify mark flags to user visible > fanotify mark flags. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > --- > fs/notify/fanotify/fanotify.h | 10 ++++++++ > fs/notify/fanotify/fanotify_user.c | 39 +++++++++++++++++------------- > fs/notify/fdinfo.c | 6 ++--- > 3 files changed, 34 insertions(+), 21 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > index a3d5b751cac5..87142bc0131a 100644 > --- a/fs/notify/fanotify/fanotify.h > +++ b/fs/notify/fanotify/fanotify.h > @@ -490,3 +490,13 @@ static inline unsigned int fanotify_event_hash_bucket( > { > return event->hash & FANOTIFY_HTABLE_MASK; > } > + > +static inline unsigned int fanotify_mark_user_flags(struct fsnotify_mark *mark) > +{ > + unsigned int mflags = 0; > + > + if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY) > + mflags |= FAN_MARK_IGNORED_SURV_MODIFY; > + > + return mflags; > +} This, together with fdinfo change should probably be a separate commit. I don't see a good reason to mix these two changes... > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 0f0db1efa379..6e78ea12239c 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1081,42 +1081,50 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group, > flags, umask); > } > > -static void fanotify_mark_add_ignored_mask(struct fsnotify_mark *fsn_mark, > - __u32 mask, unsigned int flags, > - __u32 *removed) > +static int fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark, > + unsigned int flags, bool *recalc) > { > - fsn_mark->ignored_mask |= mask; > - > /* > * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time may lead to > * the removal of the FS_MODIFY bit in calculated mask if it was set > * because of an ignored mask that is now going to survive FS_MODIFY. > */ > if ((flags & FAN_MARK_IGNORED_SURV_MODIFY) && > + (flags & FAN_MARK_IGNORED_MASK) && > !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) { > fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY; > if (!(fsn_mark->mask & FS_MODIFY)) > - *removed = FS_MODIFY; > + *recalc = true; > } > + > + return 0; > } > > -static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark, > - __u32 mask, unsigned int flags, > - __u32 *removed) > +static int fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark, > + __u32 mask, unsigned int flags) > { > - __u32 oldmask, newmask; > + __u32 oldmask; > + bool recalc = false; > + int ret; > > spin_lock(&fsn_mark->lock); > oldmask = fsnotify_calc_mask(fsn_mark); > if (!(flags & FAN_MARK_IGNORED_MASK)) { > fsn_mark->mask |= mask; > } else { > - fanotify_mark_add_ignored_mask(fsn_mark, mask, flags, removed); > + fsn_mark->ignored_mask |= mask; > } > - newmask = fsnotify_calc_mask(fsn_mark); > + > + recalc = fsnotify_calc_mask(fsn_mark) & ~oldmask & > + ~fsnotify_conn_mask(fsn_mark->connector); oldmask should be a subset of fsnotify_conn_mask() so the above should be equivalent to: recalc = fsnotify_calc_mask(fsn_mark) & ~fsnotify_conn_mask(fsn_mark->connector) shouldn't it? Otherwise this looks like a nice cleanup! Honza
On Mon, Apr 11, 2022 at 1:53 PM Jan Kara <jack@suse.cz> wrote: > > On Tue 29-03-22 10:49:00, Amir Goldstein wrote: > > Handle FAN_MARK_IGNORED_SURV_MODIFY flag change in a helper that > > is called after updating the mark mask. > > > > Move recalc of object mask inside fanotify_mark_add_to_mask() which > > makes the code a bit simpler to follow. > > > > Add also helper to translate fsnotify mark flags to user visible > > fanotify mark flags. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > --- > > fs/notify/fanotify/fanotify.h | 10 ++++++++ > > fs/notify/fanotify/fanotify_user.c | 39 +++++++++++++++++------------- > > fs/notify/fdinfo.c | 6 ++--- > > 3 files changed, 34 insertions(+), 21 deletions(-) > > > > diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h > > index a3d5b751cac5..87142bc0131a 100644 > > --- a/fs/notify/fanotify/fanotify.h > > +++ b/fs/notify/fanotify/fanotify.h > > @@ -490,3 +490,13 @@ static inline unsigned int fanotify_event_hash_bucket( > > { > > return event->hash & FANOTIFY_HTABLE_MASK; > > } > > + > > +static inline unsigned int fanotify_mark_user_flags(struct fsnotify_mark *mark) > > +{ > > + unsigned int mflags = 0; > > + > > + if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY) > > + mflags |= FAN_MARK_IGNORED_SURV_MODIFY; > > + > > + return mflags; > > +} > > This, together with fdinfo change should probably be a separate commit. I > don't see a good reason to mix these two changes... > True. > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > > index 0f0db1efa379..6e78ea12239c 100644 > > --- a/fs/notify/fanotify/fanotify_user.c > > +++ b/fs/notify/fanotify/fanotify_user.c > > @@ -1081,42 +1081,50 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group, > > flags, umask); > > } > > > > -static void fanotify_mark_add_ignored_mask(struct fsnotify_mark *fsn_mark, > > - __u32 mask, unsigned int flags, > > - __u32 *removed) > > +static int fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark, > > + unsigned int flags, bool *recalc) > > { > > - fsn_mark->ignored_mask |= mask; > > - > > /* > > * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time may lead to > > * the removal of the FS_MODIFY bit in calculated mask if it was set > > * because of an ignored mask that is now going to survive FS_MODIFY. > > */ > > if ((flags & FAN_MARK_IGNORED_SURV_MODIFY) && > > + (flags & FAN_MARK_IGNORED_MASK) && > > !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) { > > fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY; > > if (!(fsn_mark->mask & FS_MODIFY)) > > - *removed = FS_MODIFY; > > + *recalc = true; > > } > > + > > + return 0; > > } > > > > -static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark, > > - __u32 mask, unsigned int flags, > > - __u32 *removed) > > +static int fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark, > > + __u32 mask, unsigned int flags) > > { > > - __u32 oldmask, newmask; > > + __u32 oldmask; > > + bool recalc = false; > > + int ret; > > > > spin_lock(&fsn_mark->lock); > > oldmask = fsnotify_calc_mask(fsn_mark); > > if (!(flags & FAN_MARK_IGNORED_MASK)) { > > fsn_mark->mask |= mask; > > } else { > > - fanotify_mark_add_ignored_mask(fsn_mark, mask, flags, removed); > > + fsn_mark->ignored_mask |= mask; > > } > > - newmask = fsnotify_calc_mask(fsn_mark); > > + > > + recalc = fsnotify_calc_mask(fsn_mark) & ~oldmask & > > + ~fsnotify_conn_mask(fsn_mark->connector); > > oldmask should be a subset of fsnotify_conn_mask() so the above should be > equivalent to: > > recalc = fsnotify_calc_mask(fsn_mark) & ~fsnotify_conn_mask(fsn_mark->connector) > > shouldn't it? I just translated the old expression of 'added', but I guess there is no reason to look at the oldmask only the newmask matters, so I can drop oldmask variable altogether. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify.h b/fs/notify/fanotify/fanotify.h index a3d5b751cac5..87142bc0131a 100644 --- a/fs/notify/fanotify/fanotify.h +++ b/fs/notify/fanotify/fanotify.h @@ -490,3 +490,13 @@ static inline unsigned int fanotify_event_hash_bucket( { return event->hash & FANOTIFY_HTABLE_MASK; } + +static inline unsigned int fanotify_mark_user_flags(struct fsnotify_mark *mark) +{ + unsigned int mflags = 0; + + if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY) + mflags |= FAN_MARK_IGNORED_SURV_MODIFY; + + return mflags; +} diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 0f0db1efa379..6e78ea12239c 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1081,42 +1081,50 @@ static int fanotify_remove_inode_mark(struct fsnotify_group *group, flags, umask); } -static void fanotify_mark_add_ignored_mask(struct fsnotify_mark *fsn_mark, - __u32 mask, unsigned int flags, - __u32 *removed) +static int fanotify_mark_update_flags(struct fsnotify_mark *fsn_mark, + unsigned int flags, bool *recalc) { - fsn_mark->ignored_mask |= mask; - /* * Setting FAN_MARK_IGNORED_SURV_MODIFY for the first time may lead to * the removal of the FS_MODIFY bit in calculated mask if it was set * because of an ignored mask that is now going to survive FS_MODIFY. */ if ((flags & FAN_MARK_IGNORED_SURV_MODIFY) && + (flags & FAN_MARK_IGNORED_MASK) && !(fsn_mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY)) { fsn_mark->flags |= FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY; if (!(fsn_mark->mask & FS_MODIFY)) - *removed = FS_MODIFY; + *recalc = true; } + + return 0; } -static __u32 fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark, - __u32 mask, unsigned int flags, - __u32 *removed) +static int fanotify_mark_add_to_mask(struct fsnotify_mark *fsn_mark, + __u32 mask, unsigned int flags) { - __u32 oldmask, newmask; + __u32 oldmask; + bool recalc = false; + int ret; spin_lock(&fsn_mark->lock); oldmask = fsnotify_calc_mask(fsn_mark); if (!(flags & FAN_MARK_IGNORED_MASK)) { fsn_mark->mask |= mask; } else { - fanotify_mark_add_ignored_mask(fsn_mark, mask, flags, removed); + fsn_mark->ignored_mask |= mask; } - newmask = fsnotify_calc_mask(fsn_mark); + + recalc = fsnotify_calc_mask(fsn_mark) & ~oldmask & + ~fsnotify_conn_mask(fsn_mark->connector); + + ret = fanotify_mark_update_flags(fsn_mark, flags, &recalc); spin_unlock(&fsn_mark->lock); - return newmask & ~oldmask; + if (recalc) + fsnotify_recalc_mask(fsn_mark->connector); + + return ret; } static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, @@ -1174,7 +1182,6 @@ static int fanotify_add_mark(struct fsnotify_group *group, __kernel_fsid_t *fsid) { struct fsnotify_mark *fsn_mark; - __u32 added, removed = 0; int ret = 0; mutex_lock(&group->mark_mutex); @@ -1197,9 +1204,7 @@ static int fanotify_add_mark(struct fsnotify_group *group, goto out; } - added = fanotify_mark_add_to_mask(fsn_mark, mask, flags, &removed); - if (removed || (added & ~fsnotify_conn_mask(fsn_mark->connector))) - fsnotify_recalc_mask(fsn_mark->connector); + ret = fanotify_mark_add_to_mask(fsn_mark, mask, flags); out: mutex_unlock(&group->mark_mutex); diff --git a/fs/notify/fdinfo.c b/fs/notify/fdinfo.c index 754a546d647d..9f81adada3c8 100644 --- a/fs/notify/fdinfo.c +++ b/fs/notify/fdinfo.c @@ -14,6 +14,7 @@ #include <linux/exportfs.h> #include "inotify/inotify.h" +#include "fanotify/fanotify.h" #include "fdinfo.h" #include "fsnotify.h" @@ -104,12 +105,9 @@ void inotify_show_fdinfo(struct seq_file *m, struct file *f) static void fanotify_fdinfo(struct seq_file *m, struct fsnotify_mark *mark) { - unsigned int mflags = 0; + unsigned int mflags = fanotify_mark_user_flags(mark); struct inode *inode; - if (mark->flags & FSNOTIFY_MARK_FLAG_IGNORED_SURV_MODIFY) - mflags |= FAN_MARK_IGNORED_SURV_MODIFY; - if (mark->connector->type == FSNOTIFY_OBJ_TYPE_INODE) { inode = igrab(fsnotify_conn_inode(mark->connector)); if (!inode)
Handle FAN_MARK_IGNORED_SURV_MODIFY flag change in a helper that is called after updating the mark mask. Move recalc of object mask inside fanotify_mark_add_to_mask() which makes the code a bit simpler to follow. Add also helper to translate fsnotify mark flags to user visible fanotify mark flags. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify.h | 10 ++++++++ fs/notify/fanotify/fanotify_user.c | 39 +++++++++++++++++------------- fs/notify/fdinfo.c | 6 ++--- 3 files changed, 34 insertions(+), 21 deletions(-)