All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jan Kara <jack@suse.cz>
To: Amir Goldstein <amir73il@gmail.com>
Cc: Jan Kara <jack@suse.cz>, Matthew Bobrowski <repnop@google.com>,
	linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH v3 04/16] fsnotify: pass flags argument to fsnotify_add_mark() via mark
Date: Thu, 21 Apr 2022 16:18:50 +0200	[thread overview]
Message-ID: <20220421141850.e3cfr5sdiblhwvg7@quack3.lan> (raw)
In-Reply-To: <20220413090935.3127107-5-amir73il@gmail.com>

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
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

  reply	other threads:[~2022-04-21 14:35 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-13  9:09 [PATCH v3 00/16] Evictable fanotify marks Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 01/16] inotify: show inotify mask flags in proc fdinfo Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 02/16] inotify: move control flags from mask to mark flags Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 03/16] fsnotify: fix wrong lockdep annotations Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 04/16] fsnotify: pass flags argument to fsnotify_add_mark() via mark Amir Goldstein
2022-04-21 14:18   ` Jan Kara [this message]
2022-04-22 10:02     ` Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 05/16] fsnotify: pass flags argument to fsnotify_alloc_group() Amir Goldstein
2022-04-21 14:34   ` Jan Kara
2022-04-13  9:09 ` [PATCH v3 06/16] fsnotify: create helpers for group mark_mutex lock Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 07/16] inotify: use fsnotify group lock helpers Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 08/16] audit: " Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 09/16] nfsd: " Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 10/16] dnotify: " Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 11/16] fsnotify: allow adding an inode mark without pinning inode Amir Goldstein
2022-04-21 14:54   ` Jan Kara
2022-04-13  9:09 ` [PATCH v3 12/16] fanotify: create helper fanotify_mark_user_flags() Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 13/16] fanotify: factor out helper fanotify_mark_update_flags() Amir Goldstein
2022-04-21 15:00   ` Jan Kara
2022-04-13  9:09 ` [PATCH v3 14/16] fanotify: implement "evictable" inode marks Amir Goldstein
2022-04-21 15:40   ` Jan Kara
2022-04-22 10:47     ` Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 15/16] fanotify: use fsnotify group lock helpers Amir Goldstein
2022-04-13  9:09 ` [PATCH v3 16/16] fanotify: enable "evictable" inode marks Amir Goldstein
2022-04-21 15:41 ` [PATCH v3 00/16] Evictable fanotify marks Jan Kara

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220421141850.e3cfr5sdiblhwvg7@quack3.lan \
    --to=jack@suse.cz \
    --cc=amir73il@gmail.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=repnop@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.