| Message ID | 20190108164611.11440-3-jack@suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fanotify: Make wait for permission event response interruptible | expand |
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.
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
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;
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(-)