Re: [PATCH] fsnotify: Extend ioctl to allow to suppress events

[Sheng Yong <shengyong2021@gmail.com>]

Hi, Honza,

On 2021/9/17 16:49, Jan Kara wrote:
> On Thu 16-09-21 22:06:49, Sheng Yong wrote:
>> This patch adds a new ioctl cmd INOTIFY_IOC_SUPRESS to stop queueing
>> events temporarily. A second ioctl could resume queueing events again.
>> With it enabled, we could easily suppress events instead of updating
>> or removing watches, especially if we have losts of watches referring
>> to one inotify instance, for example watching a directory recursively.
>>
>> Signed-off-by: Sheng Yong <shengyong2021@gmail.com>
> 
> Thanks for the patch! This ioctl on its own is equivalent to shutting down
> the notification group so I don't think it is really useful on its own. 

Thanks for your reply.
Yes, this ioctl reuses the stop-queueing-events procedure of shutdown.

> If you add ioctl to resume queueing, it makes some sense but can you
> ellaborate a bit more why you need to stop receiving events temporarily?

The usecase is that we use rsync+inotify to backup files. Let's say there are
two tasks: 1) "real-time" task reads events and syncs the file; 2) periodic
task syncs all files recursively in the source directory.

If the queue get overflowed and new events are discarded, we have to start a
recursive sync. But recursive sync takes a very long time to scan, compare and
transmit data, we prefer a periodic task to do that, so that we could ignore
overflow events.

When the periodic task is working, I think the "real-time" task should stop
reading and handling new events temporarily to avoid duplicated IO in both
sides. I think stop queueing new events is the easiest way to do that.

thanks,
Sheng

> You'll have to rescan whole directory hierarchy anyway after resuming to be
> able to see what has changed so the usefullness of this feature is unclear
> to me...
> 
> Also we generally don't add new functionality to inotify and rather direct
> that towards fanotify and finally I'm not sure ioctl() is the best API for
> this but let's cover these questions once the usecase is clear.
> 
> 								Honza
> 
>> ---
>>   fs/notify/fanotify/fanotify_user.c |  2 +-
>>   fs/notify/group.c                  | 12 +++++++++---
>>   fs/notify/inotify/inotify_user.c   |  4 ++++
>>   fs/notify/mark.c                   |  3 ++-
>>   fs/notify/notification.c           |  4 ++--
>>   include/linux/fsnotify_backend.h   | 10 +++++++---
>>   include/uapi/linux/inotify.h       |  3 +++
>>   7 files changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/notify/fanotify/fanotify_user.c b/fs/notify/fanotify/fanotify_user.c
>> index 6facdf476255..1e24738762d3 100644
>> --- a/fs/notify/fanotify/fanotify_user.c
>> +++ b/fs/notify/fanotify/fanotify_user.c
>> @@ -773,7 +773,7 @@ static int fanotify_release(struct inode *ignored, struct file *file)
>>   	 * userspace cannot use fanotify fd anymore, no event can enter or
>>   	 * leave access_list by now either.
>>   	 */
>> -	fsnotify_group_stop_queueing(group);
>> +	fsnotify_group_stop_queueing(group, FS_GRP_SHUTDOWN);
>>   
>>   	/*
>>   	 * Process all permission events on access_list and notification queue
>> diff --git a/fs/notify/group.c b/fs/notify/group.c
>> index fb89c351295d..ce62ce6caf30 100644
>> --- a/fs/notify/group.c
>> +++ b/fs/notify/group.c
>> @@ -34,10 +34,16 @@ static void fsnotify_final_destroy_group(struct fsnotify_group *group)
>>    * Stop queueing new events for this group. Once this function returns
>>    * fsnotify_add_event() will not add any new events to the group's queue.
>>    */
>> -void fsnotify_group_stop_queueing(struct fsnotify_group *group)
>> +void fsnotify_group_stop_queueing(struct fsnotify_group *group, unsigned int st)
>>   {
>> +	if (st & ~FS_GRP_STOP_QUEUEING)
>> +		return;
>> +
>>   	spin_lock(&group->notification_lock);
>> -	group->shutdown = true;
>> +	if (group->state & st)
>> +		group->state &= ~st;
>> +	else
>> +		group->state |= st;
>>   	spin_unlock(&group->notification_lock);
>>   }
>>   
>> @@ -55,7 +61,7 @@ void fsnotify_destroy_group(struct fsnotify_group *group)
>>   	 * fsnotify_destroy_group() is called and this makes the other callers
>>   	 * of fsnotify_destroy_group() to see the same behavior.
>>   	 */
>> -	fsnotify_group_stop_queueing(group);
>> +	fsnotify_group_stop_queueing(group, FS_GRP_SHUTDOWN);
>>   
>>   	/* Clear all marks for this group and queue them for destruction */
>>   	fsnotify_clear_marks_by_group(group, FSNOTIFY_OBJ_ALL_TYPES_MASK);
>> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
>> index 62051247f6d2..67cf47f1943b 100644
>> --- a/fs/notify/inotify/inotify_user.c
>> +++ b/fs/notify/inotify/inotify_user.c
>> @@ -327,6 +327,10 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
>>   		}
>>   		break;
>>   #endif /* CONFIG_CHECKPOINT_RESTORE */
>> +	case INOTIFY_IOC_SUPPRESS:
>> +		fsnotify_group_stop_queueing(group, FS_GRP_SUPPRESS);
>> +		ret = 0;
>> +		break;
>>   	}
>>   
>>   	return ret;
>> diff --git a/fs/notify/mark.c b/fs/notify/mark.c
>> index fa1d99101f89..08f9d1e480de 100644
>> --- a/fs/notify/mark.c
>> +++ b/fs/notify/mark.c
>> @@ -343,7 +343,8 @@ static void fsnotify_put_mark_wake(struct fsnotify_mark *mark)
>>   		 * We abuse notification_waitq on group shutdown for waiting for
>>   		 * all marks pinned when waiting for userspace.
>>   		 */
>> -		if (atomic_dec_and_test(&group->user_waits) && group->shutdown)
>> +		if (atomic_dec_and_test(&group->user_waits) &&
>> +		    group->state & FS_GRP_SHUTDOWN)
>>   			wake_up(&group->notification_waitq);
>>   	}
>>   }
>> diff --git a/fs/notify/notification.c b/fs/notify/notification.c
>> index 32f45543b9c6..6586e09e9141 100644
>> --- a/fs/notify/notification.c
>> +++ b/fs/notify/notification.c
>> @@ -76,7 +76,7 @@ void fsnotify_destroy_event(struct fsnotify_group *group,
>>    * 0 if the event was added to a queue
>>    * 1 if the event was merged with some other queued event
>>    * 2 if the event was not queued - either the queue of events has overflown
>> - *   or the group is shutting down.
>> + *   or the group is suppressing or shutting down.
>>    */
>>   int fsnotify_add_event(struct fsnotify_group *group,
>>   		       struct fsnotify_event *event,
>> @@ -92,7 +92,7 @@ int fsnotify_add_event(struct fsnotify_group *group,
>>   
>>   	spin_lock(&group->notification_lock);
>>   
>> -	if (group->shutdown) {
>> +	if (group->state & FS_GRP_STOP_QUEUEING) {
>>   		spin_unlock(&group->notification_lock);
>>   		return 2;
>>   	}
>> diff --git a/include/linux/fsnotify_backend.h b/include/linux/fsnotify_backend.h
>> index 1ce66748a2d2..1f9b2afb26cb 100644
>> --- a/include/linux/fsnotify_backend.h
>> +++ b/include/linux/fsnotify_backend.h
>> @@ -103,6 +103,10 @@
>>   
>>   #define ALL_FSNOTIFY_BITS   (ALL_FSNOTIFY_EVENTS | ALL_FSNOTIFY_FLAGS)
>>   
>> +#define FS_GRP_SHUTDOWN 0x1	/* group is being shut down, don't queue more events */
>> +#define FS_GRP_SUPPRESS 0x2	/* group is being suppressed, don't queue more events */
>> +#define FS_GRP_STOP_QUEUEING (FS_GRP_SHUTDOWN | FS_GRP_SUPPRESS)
>> +
>>   struct fsnotify_group;
>>   struct fsnotify_event;
>>   struct fsnotify_mark;
>> @@ -202,7 +206,7 @@ struct fsnotify_group {
>>   	#define FS_PRIO_1	1 /* fanotify content based access control */
>>   	#define FS_PRIO_2	2 /* fanotify pre-content access */
>>   	unsigned int priority;
>> -	bool shutdown;		/* group is being shut down, don't queue more events */
>> +	unsigned int state;
>>   
>>   	/* stores all fastpath marks assoc with this group so they can be cleaned on unregister */
>>   	struct mutex mark_mutex;	/* protect marks_list */
>> @@ -472,8 +476,8 @@ extern struct fsnotify_group *fsnotify_alloc_user_group(const struct fsnotify_op
>>   extern void fsnotify_get_group(struct fsnotify_group *group);
>>   /* drop reference on a group from fsnotify_alloc_group */
>>   extern void fsnotify_put_group(struct fsnotify_group *group);
>> -/* group destruction begins, stop queuing new events */
>> -extern void fsnotify_group_stop_queueing(struct fsnotify_group *group);
>> +/* group destruction begins or suppresses, stop queuing new events */
>> +extern void fsnotify_group_stop_queueing(struct fsnotify_group *group, unsigned int st);
>>   /* destroy group */
>>   extern void fsnotify_destroy_group(struct fsnotify_group *group);
>>   /* fasync handler function */
>> diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
>> index 884b4846b630..07155241d5a9 100644
>> --- a/include/uapi/linux/inotify.h
>> +++ b/include/uapi/linux/inotify.h
>> @@ -78,7 +78,10 @@ struct inotify_event {
>>    *
>>    * INOTIFY_IOC_SETNEXTWD: set desired number of next created
>>    * watch descriptor.
>> + *
>> + * INOTIFY_IOC_SUPPRESS: suppress events temporarily.
>>    */
>>   #define INOTIFY_IOC_SETNEXTWD	_IOW('I', 0, __s32)
>> +#define INOTIFY_IOC_SUPPRESS	_IOW('I', 1, __s32)
>>   
>>   #endif /* _UAPI_LINUX_INOTIFY_H */
>> -- 
>> 2.20.1
>>