fanotify: Make wait for permission events interruptible
diff mbox series

Message ID 20190321151142.17104-1-jack@suse.cz
State New
Headers show
Series
  • fanotify: Make wait for permission events interruptible
Related show

Commit Message

Jan Kara March 21, 2019, 3:11 p.m. UTC
Switch waiting for response to fanotify permission events interruptible.
This allows e.g. the system to be suspended while there are some
fanotify permission events pending (which is reportedly pretty common
when for example AV solution is in use). However just making the wait
interruptible can result in e.g. open(2) returning -EINTR where
previously such error code never happened in practice. To avoid
confusion of userspace due to this error code, return -ERESTARTNOINTR
instead.

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

Orion, can you give this patch some testing with your usecase? Also if anybody
sees any issue with returning -ERESTARTNOINTR I have missed, please speak up.

Comments

Jan Kara April 15, 2019, 9:59 a.m. UTC | #1
On Thu 21-03-19 16:11:42, Jan Kara wrote:
> Switch waiting for response to fanotify permission events interruptible.
> This allows e.g. the system to be suspended while there are some
> fanotify permission events pending (which is reportedly pretty common
> when for example AV solution is in use). However just making the wait
> interruptible can result in e.g. open(2) returning -EINTR where
> previously such error code never happened in practice. To avoid
> confusion of userspace due to this error code, return -ERESTARTNOINTR
> instead.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/notify/fanotify/fanotify.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> Orion, can you give this patch some testing with your usecase? Also if anybody
> sees any issue with returning -ERESTARTNOINTR I have missed, please speak up.

Ping Orion? Did you have any chance to give this patch a try? Does it fix
hibernation issues you observe without causing issues with bash and other
programs? I'd like to queue this patch for the coming merge window but
I'd like to see some testing results showing that it actually helps
anything... Thanks!

								Honza

> 
> diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
> index 6b9c27548997..eb790853844b 100644
> --- a/fs/notify/fanotify/fanotify.c
> +++ b/fs/notify/fanotify/fanotify.c
> @@ -92,10 +92,17 @@ static int fanotify_get_response(struct fsnotify_group *group,
>  
>  	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
>  
> -	ret = wait_event_killable(group->fanotify_data.access_waitq,
> -				  event->state == FAN_EVENT_ANSWERED);
> +	ret = wait_event_interruptible(group->fanotify_data.access_waitq,
> +				       event->state == FAN_EVENT_ANSWERED);
>  	/* Signal pending? */
>  	if (ret < 0) {
> +		/*
> +		 * Force restarting a syscall so that this is mostly invisible
> +		 * for userspace which is not prepared for handling EINTR e.g.
> +		 * from open(2).
> +		 */
> +		if (ret == -ERESTARTSYS)
> +			ret = -ERESTARTNOINTR;
>  		spin_lock(&group->notification_lock);
>  		/* Event reported to userspace and no answer yet? */
>  		if (event->state == FAN_EVENT_REPORTED) {
> -- 
> 2.16.4
>
Orion Poplawski April 29, 2019, 4:05 p.m. UTC | #2
On 4/15/19 3:59 AM, Jan Kara wrote:
> On Thu 21-03-19 16:11:42, Jan Kara wrote:
>> Switch waiting for response to fanotify permission events interruptible.
>> This allows e.g. the system to be suspended while there are some
>> fanotify permission events pending (which is reportedly pretty common
>> when for example AV solution is in use). However just making the wait
>> interruptible can result in e.g. open(2) returning -EINTR where
>> previously such error code never happened in practice. To avoid
>> confusion of userspace due to this error code, return -ERESTARTNOINTR
>> instead.
>>
>> Signed-off-by: Jan Kara <jack@suse.cz>
>> ---
>>  fs/notify/fanotify/fanotify.c | 11 +++++++++--
>>  1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> Orion, can you give this patch some testing with your usecase? Also if anybody
>> sees any issue with returning -ERESTARTNOINTR I have missed, please speak up.
> 
> Ping Orion? Did you have any chance to give this patch a try? Does it fix
> hibernation issues you observe without causing issues with bash and other
> programs? I'd like to queue this patch for the coming merge window but
> I'd like to see some testing results showing that it actually helps
> anything... Thanks!
> 
> 								Honza


I've been running it for a while with mostly promising results but one
concern.  Notably, when running in conjuction with BitDefender Anti-Virus I
have noticed issues when cloning large git projects (seems to be a good stress
test on open()).  I believe the problems go away when BD is stopped.  At this
point I'm not sure if the issue lies more with BD or with the kernel patch.

Patch
diff mbox series

diff --git a/fs/notify/fanotify/fanotify.c b/fs/notify/fanotify/fanotify.c
index 6b9c27548997..eb790853844b 100644
--- a/fs/notify/fanotify/fanotify.c
+++ b/fs/notify/fanotify/fanotify.c
@@ -92,10 +92,17 @@  static int fanotify_get_response(struct fsnotify_group *group,
 
 	pr_debug("%s: group=%p event=%p\n", __func__, group, event);
 
-	ret = wait_event_killable(group->fanotify_data.access_waitq,
-				  event->state == FAN_EVENT_ANSWERED);
+	ret = wait_event_interruptible(group->fanotify_data.access_waitq,
+				       event->state == FAN_EVENT_ANSWERED);
 	/* Signal pending? */
 	if (ret < 0) {
+		/*
+		 * Force restarting a syscall so that this is mostly invisible
+		 * for userspace which is not prepared for handling EINTR e.g.
+		 * from open(2).
+		 */
+		if (ret == -ERESTARTSYS)
+			ret = -ERESTARTNOINTR;
 		spin_lock(&group->notification_lock);
 		/* Event reported to userspace and no answer yet? */
 		if (event->state == FAN_EVENT_REPORTED) {