[2/4] fanotify: Move locking inside get_one_event()
diff mbox series

Message ID 20190108164611.11440-3-jack@suse.cz
State New
Headers show
Series
  • fanotify: Make wait for permission event response interruptible
Related show

Commit Message

Jan Kara Jan. 8, 2019, 4:46 p.m. UTC
get_one_event() has a single caller and that just locks
notification_lock around the call. Move locking inside get_one_event()
as that will make using ->response field for permission event state
easier.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/notify/fanotify/fanotify_user.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

Comments

Amir Goldstein Jan. 9, 2019, 7:09 a.m. UTC | #1
On Tue, Jan 8, 2019 at 6:46 PM Jan Kara <jack@suse.cz> wrote:
>
> get_one_event() has a single caller and that just locks
> notification_lock around the call. Move locking inside get_one_event()
> as that will make using ->response field for permission event state
> easier.
>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify_user.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> index 908ebc421d15..2b2c8b8a17bd 100644
> --- a/fs/notify/fanotify/fanotify_user.c
> +++ b/fs/notify/fanotify/fanotify_user.c
> @@ -51,25 +51,24 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
>   * Get an fsnotify notification event if one exists and is small
>   * enough to fit in "count". Return an error pointer if the count
>   * is not large enough.
> - *
> - * Called with the group->notification_lock held.
>   */
>  static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
>                                             size_t count)
>  {
> -       assert_spin_locked(&group->notification_lock);
> -
> -       pr_debug("%s: group=%p count=%zd\n", __func__, group, count);

I see you are slowly cleaning up pr_debug calls. Any particular reason?
Out of all the spam pr_debug calls in the code, this one looks rather useful.

Otherwise, looks ok.

Thanks,
Amir.
Jan Kara Jan. 9, 2019, 9:12 a.m. UTC | #2
On Wed 09-01-19 09:09:20, Amir Goldstein wrote:
> On Tue, Jan 8, 2019 at 6:46 PM Jan Kara <jack@suse.cz> wrote:
> >
> > get_one_event() has a single caller and that just locks
> > notification_lock around the call. Move locking inside get_one_event()
> > as that will make using ->response field for permission event state
> > easier.
> >
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/notify/fanotify/fanotify_user.c | 26 +++++++++++---------------
> >  1 file changed, 11 insertions(+), 15 deletions(-)
> >
> > diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
> > index 908ebc421d15..2b2c8b8a17bd 100644
> > --- a/fs/notify/fanotify/fanotify_user.c
> > +++ b/fs/notify/fanotify/fanotify_user.c
> > @@ -51,25 +51,24 @@ struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
> >   * Get an fsnotify notification event if one exists and is small
> >   * enough to fit in "count". Return an error pointer if the count
> >   * is not large enough.
> > - *
> > - * Called with the group->notification_lock held.
> >   */
> >  static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
> >                                             size_t count)
> >  {
> > -       assert_spin_locked(&group->notification_lock);
> > -
> > -       pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
> 
> I see you are slowly cleaning up pr_debug calls. Any particular reason?
> Out of all the spam pr_debug calls in the code, this one looks rather useful.

I didn't find it useful but if you think I it, I'll leave it in.

								Honza

Patch
diff mbox series

diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
index 908ebc421d15..2b2c8b8a17bd 100644
--- a/fs/notify/fanotify/fanotify_user.c
+++ b/fs/notify/fanotify/fanotify_user.c
@@ -51,25 +51,24 @@  struct kmem_cache *fanotify_perm_event_cachep __read_mostly;
  * Get an fsnotify notification event if one exists and is small
  * enough to fit in "count". Return an error pointer if the count
  * is not large enough.
- *
- * Called with the group->notification_lock held.
  */
 static struct fsnotify_event *get_one_event(struct fsnotify_group *group,
 					    size_t count)
 {
-	assert_spin_locked(&group->notification_lock);
-
-	pr_debug("%s: group=%p count=%zd\n", __func__, group, count);
+	struct fsnotify_event *event = NULL;
 
+	spin_lock(&group->notification_lock);
 	if (fsnotify_notify_queue_is_empty(group))
-		return NULL;
+		goto out;
 
-	if (FAN_EVENT_METADATA_LEN > count)
-		return ERR_PTR(-EINVAL);
-
-	/* held the notification_lock the whole time, so this is the
-	 * same event we peeked above */
-	return fsnotify_remove_first_event(group);
+	if (FAN_EVENT_METADATA_LEN > count) {
+		event = ERR_PTR(-EINVAL);
+		goto out;
+	}
+	event = fsnotify_remove_first_event(group);
+out:
+	spin_unlock(&group->notification_lock);
+	return event;
 }
 
 static int create_fd(struct fsnotify_group *group,
@@ -261,10 +260,7 @@  static ssize_t fanotify_read(struct file *file, char __user *buf,
 
 	add_wait_queue(&group->notification_waitq, &wait);
 	while (1) {
-		spin_lock(&group->notification_lock);
 		kevent = get_one_event(group, count);
-		spin_unlock(&group->notification_lock);
-
 		if (IS_ERR(kevent)) {
 			ret = PTR_ERR(kevent);
 			break;