| Message ID | 20220413090935.3127107-5-amir73il@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Evictable fanotify marks | expand |
On Wed 13-04-22 12:09:23, Amir Goldstein wrote: > Instead of passing the allow_dups argument to fsnotify_add_mark() > as an argument, define the mark flag FSNOTIFY_MARK_FLAG_ALLOW_DUPS > to express the old allow_dups meaning and pass this information on the > mark itself. > > We use mark->flags to pass inotify control flags and will pass more > control flags in the future so let's make this the standard. > > Although the FSNOTIFY_MARK_FLAG_ALLOW_DUPS flag is not used after the > call to fsnotify_add_mark(), it does not hurt to leave this information > on the mark for future use. > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> I wanted to comment on this already last time but forgot: FSNOTIFY_MARK_FLAG_ALLOW_DUPS is IMO more a property of fsnotify_group than a particular mark (or a particular call to fsnotify_add_mark()). As such it would make more sense to me to have is as "feature" similarly to fs-reclaim restrictions you introduce later in the series. As a bonus, no need for 'flags' argument to fsnotify_add_inode_mark_locked() or fsnotify_add_inode_mark(). Honza > --- > fs/notify/fanotify/fanotify_user.c | 2 +- > fs/notify/inotify/inotify_user.c | 4 ++-- > fs/notify/mark.c | 13 ++++++------- > include/linux/fsnotify_backend.h | 19 ++++++++++--------- > kernel/audit_fsnotify.c | 3 ++- > 5 files changed, 21 insertions(+), 20 deletions(-) > > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c > index 9b32b76a9c30..0f0db1efa379 100644 > --- a/fs/notify/fanotify/fanotify_user.c > +++ b/fs/notify/fanotify/fanotify_user.c > @@ -1144,7 +1144,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, > } > > fsnotify_init_mark(mark, group); > - ret = fsnotify_add_mark_locked(mark, connp, obj_type, 0, fsid); > + ret = fsnotify_add_mark_locked(mark, connp, obj_type, fsid); > if (ret) { > fsnotify_put_mark(mark); > goto out_dec_ucounts; > diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c > index d8907d32a05b..6fc0f598a7aa 100644 > --- a/fs/notify/inotify/inotify_user.c > +++ b/fs/notify/inotify/inotify_user.c > @@ -603,7 +603,6 @@ static int inotify_new_watch(struct fsnotify_group *group, > > fsnotify_init_mark(&tmp_i_mark->fsn_mark, group); > tmp_i_mark->fsn_mark.mask = inotify_arg_to_mask(inode, arg); > - tmp_i_mark->fsn_mark.flags = inotify_arg_to_flags(arg); > tmp_i_mark->wd = -1; > > ret = inotify_add_to_idr(idr, idr_lock, tmp_i_mark); > @@ -618,7 +617,8 @@ static int inotify_new_watch(struct fsnotify_group *group, > } > > /* we are on the idr, now get on the inode */ > - ret = fsnotify_add_inode_mark_locked(&tmp_i_mark->fsn_mark, inode, 0); > + ret = fsnotify_add_inode_mark_locked(&tmp_i_mark->fsn_mark, inode, > + inotify_arg_to_flags(arg)); > if (ret) { > /* we failed to get on the inode, get off the idr */ > inotify_remove_from_idr(group, tmp_i_mark); > diff --git a/fs/notify/mark.c b/fs/notify/mark.c > index c86982be2d50..ea8f557881b1 100644 > --- a/fs/notify/mark.c > +++ b/fs/notify/mark.c > @@ -574,7 +574,7 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector( > static int fsnotify_add_mark_list(struct fsnotify_mark *mark, > fsnotify_connp_t *connp, > unsigned int obj_type, > - int allow_dups, __kernel_fsid_t *fsid) > + __kernel_fsid_t *fsid) > { > struct fsnotify_mark *lmark, *last = NULL; > struct fsnotify_mark_connector *conn; > @@ -633,7 +633,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark, > > if ((lmark->group == mark->group) && > (lmark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) && > - !allow_dups) { > + !(mark->flags & FSNOTIFY_MARK_FLAG_ALLOW_DUPS)) { > err = -EEXIST; > goto out_err; > } > @@ -668,7 +668,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark, > */ > int fsnotify_add_mark_locked(struct fsnotify_mark *mark, > fsnotify_connp_t *connp, unsigned int obj_type, > - int allow_dups, __kernel_fsid_t *fsid) > + __kernel_fsid_t *fsid) > { > struct fsnotify_group *group = mark->group; > int ret = 0; > @@ -688,7 +688,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, > fsnotify_get_mark(mark); /* for g_list */ > spin_unlock(&mark->lock); > > - ret = fsnotify_add_mark_list(mark, connp, obj_type, allow_dups, fsid); > + ret = fsnotify_add_mark_list(mark, connp, obj_type, fsid); > if (ret) > goto err; > > @@ -708,14 +708,13 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, > } > > int fsnotify_add_mark(struct fsnotify_mark *mark, fsnotify_connp_t *connp, > - unsigned int obj_type, int allow_dups, > - __kernel_fsid_t *fsid) > + unsigned int obj_type, __kernel_fsid_t *fsid) > { > int ret; > struct fsnotify_group *group = mark->group; > > mutex_lock(&group->mark_mutex); > - ret = fsnotify_add_mark_locked(mark, connp, obj_type, allow_dups, fsid); > + ret = fsnotify_add_mark_locked(mark, connp, obj_type, fsid); > mutex_unlock(&group->mark_mutex); > return ret; > } > diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h > index b1c72edd9784..2ff686882303 100644 > --- a/include/linux/fsnotify_backend.h > +++ b/include/linux/fsnotify_backend.h > @@ -473,6 +473,7 @@ struct fsnotify_mark { > /* General fsnotify mark flags */ > #define FSNOTIFY_MARK_FLAG_ALIVE 0x0001 > #define FSNOTIFY_MARK_FLAG_ATTACHED 0x0002 > +#define FSNOTIFY_MARK_FLAG_ALLOW_DUPS 0x0004 > /* inotify mark flags */ > #define FSNOTIFY_MARK_FLAG_EXCL_UNLINK 0x0010 > #define FSNOTIFY_MARK_FLAG_IN_ONESHOT 0x0020 > @@ -634,30 +635,30 @@ extern struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp, > /* Get cached fsid of filesystem containing object */ > extern int fsnotify_get_conn_fsid(const struct fsnotify_mark_connector *conn, > __kernel_fsid_t *fsid); > + > /* attach the mark to the object */ > extern int fsnotify_add_mark(struct fsnotify_mark *mark, > fsnotify_connp_t *connp, unsigned int obj_type, > - int allow_dups, __kernel_fsid_t *fsid); > + __kernel_fsid_t *fsid); > extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark, > fsnotify_connp_t *connp, > - unsigned int obj_type, int allow_dups, > + unsigned int obj_type, > __kernel_fsid_t *fsid); > > /* attach the mark to the inode */ > static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark, > - struct inode *inode, > - int allow_dups) > + struct inode *inode, int flags) > { > + mark->flags = flags; > return fsnotify_add_mark(mark, &inode->i_fsnotify_marks, > - FSNOTIFY_OBJ_TYPE_INODE, allow_dups, NULL); > + FSNOTIFY_OBJ_TYPE_INODE, NULL); > } > static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark, > - struct inode *inode, > - int allow_dups) > + struct inode *inode, int flags) > { > + mark->flags = flags; > return fsnotify_add_mark_locked(mark, &inode->i_fsnotify_marks, > - FSNOTIFY_OBJ_TYPE_INODE, allow_dups, > - NULL); > + FSNOTIFY_OBJ_TYPE_INODE, NULL); > } > > /* given a group and a mark, flag mark to be freed when all references are dropped */ > diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c > index 02348b48447c..3c35649bc7f5 100644 > --- a/kernel/audit_fsnotify.c > +++ b/kernel/audit_fsnotify.c > @@ -100,7 +100,8 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa > audit_update_mark(audit_mark, dentry->d_inode); > audit_mark->rule = krule; > > - ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, true); > + ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, > + FSNOTIFY_MARK_FLAG_ALLOW_DUPS); > if (ret < 0) { > fsnotify_put_mark(&audit_mark->mark); > audit_mark = ERR_PTR(ret); > -- > 2.35.1 >
On Thu, Apr 21, 2022 at 5:35 PM Jan Kara <jack@suse.cz> wrote: > > On Wed 13-04-22 12:09:23, Amir Goldstein wrote: > > Instead of passing the allow_dups argument to fsnotify_add_mark() > > as an argument, define the mark flag FSNOTIFY_MARK_FLAG_ALLOW_DUPS > > to express the old allow_dups meaning and pass this information on the > > mark itself. > > > > We use mark->flags to pass inotify control flags and will pass more > > control flags in the future so let's make this the standard. > > > > Although the FSNOTIFY_MARK_FLAG_ALLOW_DUPS flag is not used after the > > call to fsnotify_add_mark(), it does not hurt to leave this information > > on the mark for future use. > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com> > > I wanted to comment on this already last time but forgot: > FSNOTIFY_MARK_FLAG_ALLOW_DUPS is IMO more a property of fsnotify_group > than a particular mark (or a particular call to fsnotify_add_mark()). As > such it would make more sense to me to have is as "feature" similarly to > fs-reclaim restrictions you introduce later in the series. That's a good idea. I'll do that. > > As a bonus, no need for 'flags' argument to > fsnotify_add_inode_mark_locked() or fsnotify_add_inode_mark(). I prefer to avoid collecting this bonus and leave a flags argument for future use. The reason is that I intend to try and convince you to take the patch for FSNOTIFY_ADD_MARK_UPDATE_MASKS in a future patch set, so for the chance that I am able to convince you, let's avoid the churn for now. We can always cleanup the unneeded flags argument later. Thanks, Amir.
diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c index 9b32b76a9c30..0f0db1efa379 100644 --- a/fs/notify/fanotify/fanotify_user.c +++ b/fs/notify/fanotify/fanotify_user.c @@ -1144,7 +1144,7 @@ static struct fsnotify_mark *fanotify_add_new_mark(struct fsnotify_group *group, } fsnotify_init_mark(mark, group); - ret = fsnotify_add_mark_locked(mark, connp, obj_type, 0, fsid); + ret = fsnotify_add_mark_locked(mark, connp, obj_type, fsid); if (ret) { fsnotify_put_mark(mark); goto out_dec_ucounts; diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c index d8907d32a05b..6fc0f598a7aa 100644 --- a/fs/notify/inotify/inotify_user.c +++ b/fs/notify/inotify/inotify_user.c @@ -603,7 +603,6 @@ static int inotify_new_watch(struct fsnotify_group *group, fsnotify_init_mark(&tmp_i_mark->fsn_mark, group); tmp_i_mark->fsn_mark.mask = inotify_arg_to_mask(inode, arg); - tmp_i_mark->fsn_mark.flags = inotify_arg_to_flags(arg); tmp_i_mark->wd = -1; ret = inotify_add_to_idr(idr, idr_lock, tmp_i_mark); @@ -618,7 +617,8 @@ static int inotify_new_watch(struct fsnotify_group *group, } /* we are on the idr, now get on the inode */ - ret = fsnotify_add_inode_mark_locked(&tmp_i_mark->fsn_mark, inode, 0); + ret = fsnotify_add_inode_mark_locked(&tmp_i_mark->fsn_mark, inode, + inotify_arg_to_flags(arg)); if (ret) { /* we failed to get on the inode, get off the idr */ inotify_remove_from_idr(group, tmp_i_mark); diff --git a/fs/notify/mark.c b/fs/notify/mark.c index c86982be2d50..ea8f557881b1 100644 --- a/fs/notify/mark.c +++ b/fs/notify/mark.c @@ -574,7 +574,7 @@ static struct fsnotify_mark_connector *fsnotify_grab_connector( static int fsnotify_add_mark_list(struct fsnotify_mark *mark, fsnotify_connp_t *connp, unsigned int obj_type, - int allow_dups, __kernel_fsid_t *fsid) + __kernel_fsid_t *fsid) { struct fsnotify_mark *lmark, *last = NULL; struct fsnotify_mark_connector *conn; @@ -633,7 +633,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark, if ((lmark->group == mark->group) && (lmark->flags & FSNOTIFY_MARK_FLAG_ATTACHED) && - !allow_dups) { + !(mark->flags & FSNOTIFY_MARK_FLAG_ALLOW_DUPS)) { err = -EEXIST; goto out_err; } @@ -668,7 +668,7 @@ static int fsnotify_add_mark_list(struct fsnotify_mark *mark, */ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, fsnotify_connp_t *connp, unsigned int obj_type, - int allow_dups, __kernel_fsid_t *fsid) + __kernel_fsid_t *fsid) { struct fsnotify_group *group = mark->group; int ret = 0; @@ -688,7 +688,7 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, fsnotify_get_mark(mark); /* for g_list */ spin_unlock(&mark->lock); - ret = fsnotify_add_mark_list(mark, connp, obj_type, allow_dups, fsid); + ret = fsnotify_add_mark_list(mark, connp, obj_type, fsid); if (ret) goto err; @@ -708,14 +708,13 @@ int fsnotify_add_mark_locked(struct fsnotify_mark *mark, } int fsnotify_add_mark(struct fsnotify_mark *mark, fsnotify_connp_t *connp, - unsigned int obj_type, int allow_dups, - __kernel_fsid_t *fsid) + unsigned int obj_type, __kernel_fsid_t *fsid) { int ret; struct fsnotify_group *group = mark->group; mutex_lock(&group->mark_mutex); - ret = fsnotify_add_mark_locked(mark, connp, obj_type, allow_dups, fsid); + ret = fsnotify_add_mark_locked(mark, connp, obj_type, fsid); mutex_unlock(&group->mark_mutex); return ret; } diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h index b1c72edd9784..2ff686882303 100644 --- a/include/linux/fsnotify_backend.h +++ b/include/linux/fsnotify_backend.h @@ -473,6 +473,7 @@ struct fsnotify_mark { /* General fsnotify mark flags */ #define FSNOTIFY_MARK_FLAG_ALIVE 0x0001 #define FSNOTIFY_MARK_FLAG_ATTACHED 0x0002 +#define FSNOTIFY_MARK_FLAG_ALLOW_DUPS 0x0004 /* inotify mark flags */ #define FSNOTIFY_MARK_FLAG_EXCL_UNLINK 0x0010 #define FSNOTIFY_MARK_FLAG_IN_ONESHOT 0x0020 @@ -634,30 +635,30 @@ extern struct fsnotify_mark *fsnotify_find_mark(fsnotify_connp_t *connp, /* Get cached fsid of filesystem containing object */ extern int fsnotify_get_conn_fsid(const struct fsnotify_mark_connector *conn, __kernel_fsid_t *fsid); + /* attach the mark to the object */ extern int fsnotify_add_mark(struct fsnotify_mark *mark, fsnotify_connp_t *connp, unsigned int obj_type, - int allow_dups, __kernel_fsid_t *fsid); + __kernel_fsid_t *fsid); extern int fsnotify_add_mark_locked(struct fsnotify_mark *mark, fsnotify_connp_t *connp, - unsigned int obj_type, int allow_dups, + unsigned int obj_type, __kernel_fsid_t *fsid); /* attach the mark to the inode */ static inline int fsnotify_add_inode_mark(struct fsnotify_mark *mark, - struct inode *inode, - int allow_dups) + struct inode *inode, int flags) { + mark->flags = flags; return fsnotify_add_mark(mark, &inode->i_fsnotify_marks, - FSNOTIFY_OBJ_TYPE_INODE, allow_dups, NULL); + FSNOTIFY_OBJ_TYPE_INODE, NULL); } static inline int fsnotify_add_inode_mark_locked(struct fsnotify_mark *mark, - struct inode *inode, - int allow_dups) + struct inode *inode, int flags) { + mark->flags = flags; return fsnotify_add_mark_locked(mark, &inode->i_fsnotify_marks, - FSNOTIFY_OBJ_TYPE_INODE, allow_dups, - NULL); + FSNOTIFY_OBJ_TYPE_INODE, NULL); } /* given a group and a mark, flag mark to be freed when all references are dropped */ diff --git a/kernel/audit_fsnotify.c b/kernel/audit_fsnotify.c index 02348b48447c..3c35649bc7f5 100644 --- a/kernel/audit_fsnotify.c +++ b/kernel/audit_fsnotify.c @@ -100,7 +100,8 @@ struct audit_fsnotify_mark *audit_alloc_mark(struct audit_krule *krule, char *pa audit_update_mark(audit_mark, dentry->d_inode); audit_mark->rule = krule; - ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, true); + ret = fsnotify_add_inode_mark(&audit_mark->mark, inode, + FSNOTIFY_MARK_FLAG_ALLOW_DUPS); if (ret < 0) { fsnotify_put_mark(&audit_mark->mark); audit_mark = ERR_PTR(ret);
Instead of passing the allow_dups argument to fsnotify_add_mark() as an argument, define the mark flag FSNOTIFY_MARK_FLAG_ALLOW_DUPS to express the old allow_dups meaning and pass this information on the mark itself. We use mark->flags to pass inotify control flags and will pass more control flags in the future so let's make this the standard. Although the FSNOTIFY_MARK_FLAG_ALLOW_DUPS flag is not used after the call to fsnotify_add_mark(), it does not hurt to leave this information on the mark for future use. Signed-off-by: Amir Goldstein <amir73il@gmail.com> --- fs/notify/fanotify/fanotify_user.c | 2 +- fs/notify/inotify/inotify_user.c | 4 ++-- fs/notify/mark.c | 13 ++++++------- include/linux/fsnotify_backend.h | 19 ++++++++++--------- kernel/audit_fsnotify.c | 3 ++- 5 files changed, 21 insertions(+), 20 deletions(-)