On Sat 10-09-16 02:33:34, Lino Sanfilippo wrote:
> > @@ -379,12 +383,21 @@ static int fanotify_release(struct inode *ignored, struct file *file)
> > spin_unlock(&group->fanotify_data.access_lock);
> >
> > /*
> > - * Since bypass_perm is set, newly queued events will not wait for
> > - * access response. Wake up the already sleeping ones now.
> > - * synchronize_srcu() in fsnotify_destroy_group() will wait for all
> > - * processes sleeping in fanotify_handle_event() waiting for access
> > - * response and thus also for all permission events to be freed.
> > + * Destroy all non-permission events. For permission events just
> > + * dequeue them and set the response. They will be freed once the
> > + * response is consumed and fanotify_get_response() returns.
> > */
> > + mutex_lock(&group->notification_mutex);
> > + while (!fsnotify_notify_queue_is_empty(group)) {
> > + fsn_event = fsnotify_remove_first_event(group);
> > + if (!(fsn_event->mask & FAN_ALL_PERM_EVENTS))
> > + fsnotify_destroy_event(group, fsn_event);
> > + else
> > + FANOTIFY_PE(fsn_event)->response = FAN_ALLOW;
>
> Why do we have to handle (i.e destroy) non-permission events here? Is this
> meant as some kind of optimization?
Where else would they be destroyed? Normal events are destroyed when
userspace reads them but nobody is going to read these anymore. For
permission events things are more complex - they get destroyed by the
process creating these events since that process waits for the response.
> Also, we now set the response during both iterations. I assume the
> assignment during the iteration of the access list is only a leftover and
> can be skipped.
Both are currently needed because I've changed fanotify_get_response() to
check only the response value.
> Why dont we set the shutdown flag directly in release() as soon as we
> grep the notification mutex. I dont see any reason to do this seperated
> from the removal of events from the notification list (or do I miss
> something?). Similar situation in destroy_group(): We can set the flag
> in flush_notify instead of destroy_group (we then do not even need the
> dedicated function stop_queueing() then).
We could do it like that but I didn't want fanotify to hook into generic
fsnotify internals and rather wanted to encapsulate the functionality in
a function. Since this is not really performance critical, extra round trip
on notification_mutex should be fine.
> fsnotify_remove_event() is not used any more now, so we should remove
> this function.
Yeah, will fix that.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
