[PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

Watch descriptor is id of the watch created by inotify_add_watch().
It is allocated in inotify_add_to_idr(), and takes the numbers
starting from 1. Every new inotify watch obtains next available
number (usually, old + 1), as served by idr_alloc_cyclic().

CRIU (Checkpoint/Restore In Userspace) project supports inotify
files, and restores watched descriptors with the same numbers,
they had before dump. Since there was no kernel support, we
had to use cycle to add a watch with specific descriptor id:

	while (1) {
		int wd;

		wd = inotify_add_watch(inotify_fd, path, mask);
		if (wd < 0) {
			break;
		} else if (wd == desired_wd_id) {
			ret = 0;
			break;
		}

		inotify_rm_watch(inotify_fd, wd);
	}

(You may find the actual code at the below link:
 https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)

The cycle is suboptiomal and very expensive, but since there is no better
kernel support, it was the only way to restore that. Happily, we had met
mostly descriptors with small id, and this approach had worked somehow.

But recent time containers with inotify with big watch descriptors
begun to come, and this way stopped to work at all. When descriptor id
is something about 0x34d71d6, the restoring process spins in busy loop
for a long time, and the restore hungs and delay of migration from node
to node could easily be watched.

This patch aims to solve this problem. It introduces new ioctl
INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
watch descriptor from userspace. It simply calls idr_set_cursor() primitive
to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
will return this id, if it is not occupied. This is the way which is
used to restore some other resources from userspace. For example,
/proc/sys/kernel/ns_last_pid works the same for task pids.

The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
may exclude it. The only change in generic inotify part is idr_alloc_cyclic()
end argument. We had 0 there, and idr subsystem replaced it with INT_MAX
in idr_get_free(). So, the max possible id was INT_MAX (see idr_get_free()
again).

Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
it's defined as positive number. But when not-zero value is passed
to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
defined @end as int argument. So, it's impossible to pass positive @end
argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
will be unavailable.

Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
---
 fs/notify/inotify/inotify_user.c |   19 ++++++++++++++++++-
 include/uapi/linux/inotify.h     |    8 ++++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5c29bf16814f..3c824e252c84 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -44,6 +44,9 @@
 
 #include <asm/ioctls.h>
 
+#define INOTIFY_IDR_START	1
+#define INOTIFY_IDR_END		S32_MAX
+
 /* configurable via /proc/sys/fs/inotify/ */
 static int inotify_max_queued_events __read_mostly;
 
@@ -285,6 +288,7 @@ static int inotify_release(struct inode *ignored, struct file *file)
 static long inotify_ioctl(struct file *file, unsigned int cmd,
 			  unsigned long arg)
 {
+	struct inotify_group_private_data *data __maybe_unused;
 	struct fsnotify_group *group;
 	struct fsnotify_event *fsn_event;
 	void __user *p;
@@ -293,6 +297,7 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
 
 	group = file->private_data;
 	p = (void __user *) arg;
+	data = &group->inotify_data;
 
 	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
 
@@ -307,6 +312,17 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
 		spin_unlock(&group->notification_lock);
 		ret = put_user(send_len, (int __user *) p);
 		break;
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	case INOTIFY_IOC_SETNEXTWD:
+		ret = -EINVAL;
+		if (arg >= INOTIFY_IDR_START && arg < INOTIFY_IDR_END) {
+			spin_lock(&data->idr_lock);
+			idr_set_cursor(&data->idr, (unsigned int)arg);
+			spin_unlock(&data->idr_lock);
+			ret = 0;
+		}
+		break;
+#endif /* CONFIG_CHECKPOINT_RESTORE */
 	}
 
 	return ret;
@@ -349,7 +365,8 @@ static int inotify_add_to_idr(struct idr *idr, spinlock_t *idr_lock,
 	idr_preload(GFP_KERNEL);
 	spin_lock(idr_lock);
 
-	ret = idr_alloc_cyclic(idr, i_mark, 1, 0, GFP_NOWAIT);
+	ret = idr_alloc_cyclic(idr, i_mark, INOTIFY_IDR_START,
+			       INOTIFY_IDR_END, GFP_NOWAIT);
 	if (ret >= 0) {
 		/* we added the mark to the idr, take a reference */
 		i_mark->wd = ret;
diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
index 5474461683db..245489342c14 100644
--- a/include/uapi/linux/inotify.h
+++ b/include/uapi/linux/inotify.h
@@ -71,5 +71,13 @@ struct inotify_event {
 #define IN_CLOEXEC O_CLOEXEC
 #define IN_NONBLOCK O_NONBLOCK
 
+/*
+ * ioctl numbers: inotify uses 'I' prefix for all ioctls,
+ * except historical FIONREAD, which based on 'T'.
+ *
+ * INOTIFY_IOC_SETNEXTWD: set desired number of next created
+ * watch descriptor.
+ */
+#define INOTIFY_IOC_SETNEXTWD	_IOW('I', 0, __s32)
 
 #endif /* _UAPI_LINUX_INOTIFY_H */

Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Matthew Wilcox]

On Thu, Feb 08, 2018 at 06:07:37PM +0300, Kirill Tkhai wrote:
> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
> may exclude it. The only change in generic inotify part is idr_alloc_cyclic()
> end argument. We had 0 there, and idr subsystem replaced it with INT_MAX
> in idr_get_free(). So, the max possible id was INT_MAX (see idr_get_free()
> again).
> 
> Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
> it's defined as positive number. But when not-zero value is passed
> to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
> defined @end as int argument. So, it's impossible to pass positive @end
> argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
> inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
> will be unavailable.

Ummm.  Why not just do:

+#ifdef CONFIG_CHECKPOINT_RESTORE
+	case INOTIFY_IOC_SETNEXTWD:
+		ret = -EINVAL;
+		if (arg >= 1 && arg <= INT_MAX) {
+			spin_lock(&data->idr_lock);
+			idr_set_cursor(&data->idr, (unsigned int)arg);
+			spin_unlock(&data->idr_lock);
+			ret = 0;
+		}
+		break;
+#endif /* CONFIG_CHECKPOINT_RESTORE */

Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

On 08.02.2018 19:02, Matthew Wilcox wrote:
> On Thu, Feb 08, 2018 at 06:07:37PM +0300, Kirill Tkhai wrote:
>> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
>> may exclude it. The only change in generic inotify part is idr_alloc_cyclic()
>> end argument. We had 0 there, and idr subsystem replaced it with INT_MAX
>> in idr_get_free(). So, the max possible id was INT_MAX (see idr_get_free()
>> again).
>>
>> Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
>> it's defined as positive number. But when not-zero value is passed
>> to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
>> defined @end as int argument. So, it's impossible to pass positive @end
>> argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
>> inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
>> will be unavailable.
> 
> Ummm.  Why not just do:
> 
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +	case INOTIFY_IOC_SETNEXTWD:
> +		ret = -EINVAL;
> +		if (arg >= 1 && arg <= INT_MAX) {
> +			spin_lock(&data->idr_lock);
> +			idr_set_cursor(&data->idr, (unsigned int)arg);
> +			spin_unlock(&data->idr_lock);
> +			ret = 0;
> +		}
> +		break;
> +#endif /* CONFIG_CHECKPOINT_RESTORE */

INT_MAX is generic constant, and the fact, it is used in the above hunk,
does not make feel it's the same constant as used in inotify_add_to_idr().

Specific constant underlines these two places are connected, and it makes
the code more readable for a reader.

Kirill

Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Matthew Wilcox]

On Fri, Feb 09, 2018 at 11:26:01AM +0300, Kirill Tkhai wrote:
> On 08.02.2018 19:02, Matthew Wilcox wrote:
> > On Thu, Feb 08, 2018 at 06:07:37PM +0300, Kirill Tkhai wrote:
> >> Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
> >> it's defined as positive number. But when not-zero value is passed
> >> to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
> >> defined @end as int argument. So, it's impossible to pass positive @end
> >> argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
> >> inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
> >> will be unavailable.
> > 
> > Ummm.  Why not just do:
> > 
> > +#ifdef CONFIG_CHECKPOINT_RESTORE
> > +	case INOTIFY_IOC_SETNEXTWD:
> > +		ret = -EINVAL;
> > +		if (arg >= 1 && arg <= INT_MAX) {
> > +			spin_lock(&data->idr_lock);
> > +			idr_set_cursor(&data->idr, (unsigned int)arg);
> > +			spin_unlock(&data->idr_lock);
> > +			ret = 0;
> > +		}
> > +		break;
> > +#endif /* CONFIG_CHECKPOINT_RESTORE */
> 
> INT_MAX is generic constant, and the fact, it is used in the above hunk,
> does not make feel it's the same constant as used in inotify_add_to_idr().

But the IDR guarantees you a number between 0 and INT_MAX.  If the
reader doesn't understand that, then let them find it out.  They'll be
better off for knowing that.

I improved the documentation for idr_alloc recently, although Linus hasn't
pulled it yet.  Now it reads:

/**
 * idr_alloc() - Allocate an ID.
 * @idr: IDR handle.
 * @ptr: Pointer to be associated with the new ID.
 * @start: The minimum ID (inclusive).
 * @end: The maximum ID (exclusive).
 * @gfp: Memory allocation flags.
 *
 * Allocates an unused ID in the range specified by @start and @end.  If
 * @end is <= 0, it is treated as one larger than %INT_MAX.  This allows
 * callers to use @start + N as @end as long as N is within integer range.
 *
 * The caller should provide their own locking to ensure that two
 * concurrent modifications to the IDR are not possible.  Read-only
 * accesses to the IDR may be done under the RCU read lock or may
 * exclude simultaneous writers.
 *
 * Return: The newly allocated ID, -ENOMEM if memory allocation failed,
 * or -ENOSPC if no free IDs could be found.
 */

Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

On 09.02.2018 16:28, Matthew Wilcox wrote:
> On Fri, Feb 09, 2018 at 11:26:01AM +0300, Kirill Tkhai wrote:
>> On 08.02.2018 19:02, Matthew Wilcox wrote:
>>> On Thu, Feb 08, 2018 at 06:07:37PM +0300, Kirill Tkhai wrote:
>>>> Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
>>>> it's defined as positive number. But when not-zero value is passed
>>>> to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
>>>> defined @end as int argument. So, it's impossible to pass positive @end
>>>> argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
>>>> inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
>>>> will be unavailable.
>>>
>>> Ummm.  Why not just do:
>>>
>>> +#ifdef CONFIG_CHECKPOINT_RESTORE
>>> +	case INOTIFY_IOC_SETNEXTWD:
>>> +		ret = -EINVAL;
>>> +		if (arg >= 1 && arg <= INT_MAX) {
>>> +			spin_lock(&data->idr_lock);
>>> +			idr_set_cursor(&data->idr, (unsigned int)arg);
>>> +			spin_unlock(&data->idr_lock);
>>> +			ret = 0;
>>> +		}
>>> +		break;
>>> +#endif /* CONFIG_CHECKPOINT_RESTORE */
>>
>> INT_MAX is generic constant, and the fact, it is used in the above hunk,
>> does not make feel it's the same constant as used in inotify_add_to_idr().
> 
> But the IDR guarantees you a number between 0 and INT_MAX.  If the
> reader doesn't understand that, then let them find it out.  They'll be
> better off for knowing that.
> 
> I improved the documentation for idr_alloc recently, although Linus hasn't
> pulled it yet.  Now it reads:
> 
> /**
>  * idr_alloc() - Allocate an ID.
>  * @idr: IDR handle.
>  * @ptr: Pointer to be associated with the new ID.
>  * @start: The minimum ID (inclusive).
>  * @end: The maximum ID (exclusive).
>  * @gfp: Memory allocation flags.
>  *
>  * Allocates an unused ID in the range specified by @start and @end.  If
>  * @end is <= 0, it is treated as one larger than %INT_MAX.  This allows
>  * callers to use @start + N as @end as long as N is within integer range.
>  *
>  * The caller should provide their own locking to ensure that two
>  * concurrent modifications to the IDR are not possible.  Read-only
>  * accesses to the IDR may be done under the RCU read lock or may
>  * exclude simultaneous writers.
>  *
>  * Return: The newly allocated ID, -ENOMEM if memory allocation failed,
>  * or -ENOSPC if no free IDs could be found.
>  */

I've sent v2 with INT_MAX instead of custom #define.

Kirill

Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Jan Kara]

[Added CC to linux-api]

On Thu 08-02-18 18:07:37, Kirill Tkhai wrote:
> Watch descriptor is id of the watch created by inotify_add_watch().
> It is allocated in inotify_add_to_idr(), and takes the numbers
> starting from 1. Every new inotify watch obtains next available
> number (usually, old + 1), as served by idr_alloc_cyclic().
> 
> CRIU (Checkpoint/Restore In Userspace) project supports inotify
> files, and restores watched descriptors with the same numbers,
> they had before dump. Since there was no kernel support, we
> had to use cycle to add a watch with specific descriptor id:
> 
> 	while (1) {
> 		int wd;
> 
> 		wd = inotify_add_watch(inotify_fd, path, mask);
> 		if (wd < 0) {
> 			break;
> 		} else if (wd == desired_wd_id) {
> 			ret = 0;
> 			break;
> 		}
> 
> 		inotify_rm_watch(inotify_fd, wd);
> 	}
> 
> (You may find the actual code at the below link:
>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
> 
> The cycle is suboptiomal and very expensive, but since there is no better
> kernel support, it was the only way to restore that. Happily, we had met
> mostly descriptors with small id, and this approach had worked somehow.
> 
> But recent time containers with inotify with big watch descriptors
> begun to come, and this way stopped to work at all. When descriptor id
> is something about 0x34d71d6, the restoring process spins in busy loop
> for a long time, and the restore hungs and delay of migration from node
> to node could easily be watched.
> 
> This patch aims to solve this problem. It introduces new ioctl
> INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
> watch descriptor from userspace. It simply calls idr_set_cursor() primitive
> to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
> will return this id, if it is not occupied. This is the way which is
> used to restore some other resources from userspace. For example,
> /proc/sys/kernel/ns_last_pid works the same for task pids.
> 
> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
> may exclude it. The only change in generic inotify part is idr_alloc_cyclic()
> end argument. We had 0 there, and idr subsystem replaced it with INT_MAX
> in idr_get_free(). So, the max possible id was INT_MAX (see idr_get_free()
> again).
> 
> Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
> it's defined as positive number. But when not-zero value is passed
> to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
> defined @end as int argument. So, it's impossible to pass positive @end
> argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
> inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
> will be unavailable.
> 
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>

I'm not thrilled by this but OTOH I also don't know about a better option.
The patch as such looks OK to me so unless someone objects I'll take it to
my tree in a few days.

								Honza


> ---
>  fs/notify/inotify/inotify_user.c |   19 ++++++++++++++++++-
>  include/uapi/linux/inotify.h     |    8 ++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 5c29bf16814f..3c824e252c84 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -44,6 +44,9 @@
>  
>  #include <asm/ioctls.h>
>  
> +#define INOTIFY_IDR_START	1
> +#define INOTIFY_IDR_END		S32_MAX
> +
>  /* configurable via /proc/sys/fs/inotify/ */
>  static int inotify_max_queued_events __read_mostly;
>  
> @@ -285,6 +288,7 @@ static int inotify_release(struct inode *ignored, struct file *file)
>  static long inotify_ioctl(struct file *file, unsigned int cmd,
>  			  unsigned long arg)
>  {
> +	struct inotify_group_private_data *data __maybe_unused;
>  	struct fsnotify_group *group;
>  	struct fsnotify_event *fsn_event;
>  	void __user *p;
> @@ -293,6 +297,7 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
>  
>  	group = file->private_data;
>  	p = (void __user *) arg;
> +	data = &group->inotify_data;
>  
>  	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
>  
> @@ -307,6 +312,17 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
>  		spin_unlock(&group->notification_lock);
>  		ret = put_user(send_len, (int __user *) p);
>  		break;
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +	case INOTIFY_IOC_SETNEXTWD:
> +		ret = -EINVAL;
> +		if (arg >= INOTIFY_IDR_START && arg < INOTIFY_IDR_END) {
> +			spin_lock(&data->idr_lock);
> +			idr_set_cursor(&data->idr, (unsigned int)arg);
> +			spin_unlock(&data->idr_lock);
> +			ret = 0;
> +		}
> +		break;
> +#endif /* CONFIG_CHECKPOINT_RESTORE */
>  	}
>  
>  	return ret;
> @@ -349,7 +365,8 @@ static int inotify_add_to_idr(struct idr *idr, spinlock_t *idr_lock,
>  	idr_preload(GFP_KERNEL);
>  	spin_lock(idr_lock);
>  
> -	ret = idr_alloc_cyclic(idr, i_mark, 1, 0, GFP_NOWAIT);
> +	ret = idr_alloc_cyclic(idr, i_mark, INOTIFY_IDR_START,
> +			       INOTIFY_IDR_END, GFP_NOWAIT);
>  	if (ret >= 0) {
>  		/* we added the mark to the idr, take a reference */
>  		i_mark->wd = ret;
> diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
> index 5474461683db..245489342c14 100644
> --- a/include/uapi/linux/inotify.h
> +++ b/include/uapi/linux/inotify.h
> @@ -71,5 +71,13 @@ struct inotify_event {
>  #define IN_CLOEXEC O_CLOEXEC
>  #define IN_NONBLOCK O_NONBLOCK
>  
> +/*
> + * ioctl numbers: inotify uses 'I' prefix for all ioctls,
> + * except historical FIONREAD, which based on 'T'.
> + *
> + * INOTIFY_IOC_SETNEXTWD: set desired number of next created
> + * watch descriptor.
> + */
> +#define INOTIFY_IOC_SETNEXTWD	_IOW('I', 0, __s32)
>  
>  #endif /* _UAPI_LINUX_INOTIFY_H */
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Jan Kara]

[Added CC to linux-api]

On Thu 08-02-18 18:07:37, Kirill Tkhai wrote:
> Watch descriptor is id of the watch created by inotify_add_watch().
> It is allocated in inotify_add_to_idr(), and takes the numbers
> starting from 1. Every new inotify watch obtains next available
> number (usually, old + 1), as served by idr_alloc_cyclic().
> 
> CRIU (Checkpoint/Restore In Userspace) project supports inotify
> files, and restores watched descriptors with the same numbers,
> they had before dump. Since there was no kernel support, we
> had to use cycle to add a watch with specific descriptor id:
> 
> 	while (1) {
> 		int wd;
> 
> 		wd = inotify_add_watch(inotify_fd, path, mask);
> 		if (wd < 0) {
> 			break;
> 		} else if (wd == desired_wd_id) {
> 			ret = 0;
> 			break;
> 		}
> 
> 		inotify_rm_watch(inotify_fd, wd);
> 	}
> 
> (You may find the actual code at the below link:
>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
> 
> The cycle is suboptiomal and very expensive, but since there is no better
> kernel support, it was the only way to restore that. Happily, we had met
> mostly descriptors with small id, and this approach had worked somehow.
> 
> But recent time containers with inotify with big watch descriptors
> begun to come, and this way stopped to work at all. When descriptor id
> is something about 0x34d71d6, the restoring process spins in busy loop
> for a long time, and the restore hungs and delay of migration from node
> to node could easily be watched.
> 
> This patch aims to solve this problem. It introduces new ioctl
> INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
> watch descriptor from userspace. It simply calls idr_set_cursor() primitive
> to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
> will return this id, if it is not occupied. This is the way which is
> used to restore some other resources from userspace. For example,
> /proc/sys/kernel/ns_last_pid works the same for task pids.
> 
> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
> may exclude it. The only change in generic inotify part is idr_alloc_cyclic()
> end argument. We had 0 there, and idr subsystem replaced it with INT_MAX
> in idr_get_free(). So, the max possible id was INT_MAX (see idr_get_free()
> again).
> 
> Since I need INOTIFY_IDR_END to check ioctl's third argument, it's better
> it's defined as positive number. But when not-zero value is passed
> to idr_get_free(), this function decrements it. Also, idr_alloc_cyclic()
> defined @end as int argument. So, it's impossible to pass positive @end
> argument to idr_alloc_cyclic() to get INT_MAX id. And after this patch
> inotify watch descriptors ids will take numbers [1, INT_MAX-1], INT_MAX
> will be unavailable.
> 
> Signed-off-by: Kirill Tkhai <ktkhai-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>

I'm not thrilled by this but OTOH I also don't know about a better option.
The patch as such looks OK to me so unless someone objects I'll take it to
my tree in a few days.

								Honza


> ---
>  fs/notify/inotify/inotify_user.c |   19 ++++++++++++++++++-
>  include/uapi/linux/inotify.h     |    8 ++++++++
>  2 files changed, 26 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
> index 5c29bf16814f..3c824e252c84 100644
> --- a/fs/notify/inotify/inotify_user.c
> +++ b/fs/notify/inotify/inotify_user.c
> @@ -44,6 +44,9 @@
>  
>  #include <asm/ioctls.h>
>  
> +#define INOTIFY_IDR_START	1
> +#define INOTIFY_IDR_END		S32_MAX
> +
>  /* configurable via /proc/sys/fs/inotify/ */
>  static int inotify_max_queued_events __read_mostly;
>  
> @@ -285,6 +288,7 @@ static int inotify_release(struct inode *ignored, struct file *file)
>  static long inotify_ioctl(struct file *file, unsigned int cmd,
>  			  unsigned long arg)
>  {
> +	struct inotify_group_private_data *data __maybe_unused;
>  	struct fsnotify_group *group;
>  	struct fsnotify_event *fsn_event;
>  	void __user *p;
> @@ -293,6 +297,7 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
>  
>  	group = file->private_data;
>  	p = (void __user *) arg;
> +	data = &group->inotify_data;
>  
>  	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
>  
> @@ -307,6 +312,17 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
>  		spin_unlock(&group->notification_lock);
>  		ret = put_user(send_len, (int __user *) p);
>  		break;
> +#ifdef CONFIG_CHECKPOINT_RESTORE
> +	case INOTIFY_IOC_SETNEXTWD:
> +		ret = -EINVAL;
> +		if (arg >= INOTIFY_IDR_START && arg < INOTIFY_IDR_END) {
> +			spin_lock(&data->idr_lock);
> +			idr_set_cursor(&data->idr, (unsigned int)arg);
> +			spin_unlock(&data->idr_lock);
> +			ret = 0;
> +		}
> +		break;
> +#endif /* CONFIG_CHECKPOINT_RESTORE */
>  	}
>  
>  	return ret;
> @@ -349,7 +365,8 @@ static int inotify_add_to_idr(struct idr *idr, spinlock_t *idr_lock,
>  	idr_preload(GFP_KERNEL);
>  	spin_lock(idr_lock);
>  
> -	ret = idr_alloc_cyclic(idr, i_mark, 1, 0, GFP_NOWAIT);
> +	ret = idr_alloc_cyclic(idr, i_mark, INOTIFY_IDR_START,
> +			       INOTIFY_IDR_END, GFP_NOWAIT);
>  	if (ret >= 0) {
>  		/* we added the mark to the idr, take a reference */
>  		i_mark->wd = ret;
> diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
> index 5474461683db..245489342c14 100644
> --- a/include/uapi/linux/inotify.h
> +++ b/include/uapi/linux/inotify.h
> @@ -71,5 +71,13 @@ struct inotify_event {
>  #define IN_CLOEXEC O_CLOEXEC
>  #define IN_NONBLOCK O_NONBLOCK
>  
> +/*
> + * ioctl numbers: inotify uses 'I' prefix for all ioctls,
> + * except historical FIONREAD, which based on 'T'.
> + *
> + * INOTIFY_IOC_SETNEXTWD: set desired number of next created
> + * watch descriptor.
> + */
> +#define INOTIFY_IOC_SETNEXTWD	_IOW('I', 0, __s32)
>  
>  #endif /* _UAPI_LINUX_INOTIFY_H */
> 
-- 
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR

Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor

[Cyrill Gorcunov]

On Thu, Feb 08, 2018 at 05:14:00PM +0100, Jan Kara wrote:
> > 
> > Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> 
> I'm not thrilled by this but OTOH I also don't know about a better option.
> The patch as such looks OK to me so unless someone objects I'll take it to
> my tree in a few days.

Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>

[PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

Watch descriptor is id of the watch created by inotify_add_watch().
It is allocated in inotify_add_to_idr(), and takes the numbers
starting from 1. Every new inotify watch obtains next available
number (usually, old + 1), as served by idr_alloc_cyclic().

CRIU (Checkpoint/Restore In Userspace) project supports inotify
files, and restores watched descriptors with the same numbers,
they had before dump. Since there was no kernel support, we
had to use cycle to add a watch with specific descriptor id:

	while (1) {
		int wd;

		wd = inotify_add_watch(inotify_fd, path, mask);
		if (wd < 0) {
			break;
		} else if (wd == desired_wd_id) {
			ret = 0;
			break;
		}

		inotify_rm_watch(inotify_fd, wd);
	}

(You may find the actual code at the below link:
 https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)

The cycle is suboptiomal and very expensive, but since there is no better
kernel support, it was the only way to restore that. Happily, we had met
mostly descriptors with small id, and this approach had worked somehow.

But recent time containers with inotify with big watch descriptors
begun to come, and this way stopped to work at all. When descriptor id
is something about 0x34d71d6, the restoring process spins in busy loop
for a long time, and the restore hungs and delay of migration from node
to node could easily be watched.

This patch aims to solve this problem. It introduces new ioctl
INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
watch descriptor from userspace. It simply calls idr_set_cursor() primitive
to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
will return this id, if it is not occupied. This is the way which is
used to restore some other resources from userspace. For example,
/proc/sys/kernel/ns_last_pid works the same for task pids.

The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
may exclude it.

v2: Use INT_MAX instead of custom definition of max id,
as IDR subsystem guarantees id is between 0 and INT_MAX.

CC: Jan Kara <jack@suse.cz>
CC: Matthew Wilcox <willy@infradead.org>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: Amir Goldstein <amir73il@gmail.com>
Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>
---
 fs/notify/inotify/inotify_user.c |   13 +++++++++++++
 include/uapi/linux/inotify.h     |    8 ++++++++
 2 files changed, 21 insertions(+)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5c29bf16814f..2a5b806afd09 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -285,6 +285,7 @@ static int inotify_release(struct inode *ignored, struct file *file)
 static long inotify_ioctl(struct file *file, unsigned int cmd,
 			  unsigned long arg)
 {
+	struct inotify_group_private_data *data __maybe_unused;
 	struct fsnotify_group *group;
 	struct fsnotify_event *fsn_event;
 	void __user *p;
@@ -293,6 +294,7 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
 
 	group = file->private_data;
 	p = (void __user *) arg;
+	data = &group->inotify_data;
 
 	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
 
@@ -307,6 +309,17 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
 		spin_unlock(&group->notification_lock);
 		ret = put_user(send_len, (int __user *) p);
 		break;
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	case INOTIFY_IOC_SETNEXTWD:
+		ret = -EINVAL;
+		if (arg >= 1 && arg <= INT_MAX) {
+			spin_lock(&data->idr_lock);
+			idr_set_cursor(&data->idr, (unsigned int)arg);
+			spin_unlock(&data->idr_lock);
+			ret = 0;
+		}
+		break;
+#endif /* CONFIG_CHECKPOINT_RESTORE */
 	}
 
 	return ret;
diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
index 5474461683db..4800bf2a531d 100644
--- a/include/uapi/linux/inotify.h
+++ b/include/uapi/linux/inotify.h
@@ -71,5 +71,13 @@ struct inotify_event {
 #define IN_CLOEXEC O_CLOEXEC
 #define IN_NONBLOCK O_NONBLOCK
 
+/*
+ * ioctl numbers: inotify uses 'I' prefix for all ioctls,
+ * except historical FIONREAD, which is based on 'T'.
+ *
+ * INOTIFY_IOC_SETNEXTWD: set desired number of next created
+ * watch descriptor.
+ */
+#define INOTIFY_IOC_SETNEXTWD	_IOW('I', 0, __s32)
 
 #endif /* _UAPI_LINUX_INOTIFY_H */

[PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

Watch descriptor is id of the watch created by inotify_add_watch().
It is allocated in inotify_add_to_idr(), and takes the numbers
starting from 1. Every new inotify watch obtains next available
number (usually, old + 1), as served by idr_alloc_cyclic().

CRIU (Checkpoint/Restore In Userspace) project supports inotify
files, and restores watched descriptors with the same numbers,
they had before dump. Since there was no kernel support, we
had to use cycle to add a watch with specific descriptor id:

	while (1) {
		int wd;

		wd = inotify_add_watch(inotify_fd, path, mask);
		if (wd < 0) {
			break;
		} else if (wd == desired_wd_id) {
			ret = 0;
			break;
		}

		inotify_rm_watch(inotify_fd, wd);
	}

(You may find the actual code at the below link:
 https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)

The cycle is suboptiomal and very expensive, but since there is no better
kernel support, it was the only way to restore that. Happily, we had met
mostly descriptors with small id, and this approach had worked somehow.

But recent time containers with inotify with big watch descriptors
begun to come, and this way stopped to work at all. When descriptor id
is something about 0x34d71d6, the restoring process spins in busy loop
for a long time, and the restore hungs and delay of migration from node
to node could easily be watched.

This patch aims to solve this problem. It introduces new ioctl
INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
watch descriptor from userspace. It simply calls idr_set_cursor() primitive
to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
will return this id, if it is not occupied. This is the way which is
used to restore some other resources from userspace. For example,
/proc/sys/kernel/ns_last_pid works the same for task pids.

The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
may exclude it.

v2: Use INT_MAX instead of custom definition of max id,
as IDR subsystem guarantees id is between 0 and INT_MAX.

CC: Jan Kara <jack-AlSwsSmVLrQ@public.gmane.org>
CC: Matthew Wilcox <willy-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
CC: Amir Goldstein <amir73il-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Kirill Tkhai <ktkhai-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>
Reviewed-by: Cyrill Gorcunov <gorcunov-GEFAQzZX7r8dnm+yROfE0A@public.gmane.org>
---
 fs/notify/inotify/inotify_user.c |   13 +++++++++++++
 include/uapi/linux/inotify.h     |    8 ++++++++
 2 files changed, 21 insertions(+)

diff --git a/fs/notify/inotify/inotify_user.c b/fs/notify/inotify/inotify_user.c
index 5c29bf16814f..2a5b806afd09 100644
--- a/fs/notify/inotify/inotify_user.c
+++ b/fs/notify/inotify/inotify_user.c
@@ -285,6 +285,7 @@ static int inotify_release(struct inode *ignored, struct file *file)
 static long inotify_ioctl(struct file *file, unsigned int cmd,
 			  unsigned long arg)
 {
+	struct inotify_group_private_data *data __maybe_unused;
 	struct fsnotify_group *group;
 	struct fsnotify_event *fsn_event;
 	void __user *p;
@@ -293,6 +294,7 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
 
 	group = file->private_data;
 	p = (void __user *) arg;
+	data = &group->inotify_data;
 
 	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
 
@@ -307,6 +309,17 @@ static long inotify_ioctl(struct file *file, unsigned int cmd,
 		spin_unlock(&group->notification_lock);
 		ret = put_user(send_len, (int __user *) p);
 		break;
+#ifdef CONFIG_CHECKPOINT_RESTORE
+	case INOTIFY_IOC_SETNEXTWD:
+		ret = -EINVAL;
+		if (arg >= 1 && arg <= INT_MAX) {
+			spin_lock(&data->idr_lock);
+			idr_set_cursor(&data->idr, (unsigned int)arg);
+			spin_unlock(&data->idr_lock);
+			ret = 0;
+		}
+		break;
+#endif /* CONFIG_CHECKPOINT_RESTORE */
 	}
 
 	return ret;
diff --git a/include/uapi/linux/inotify.h b/include/uapi/linux/inotify.h
index 5474461683db..4800bf2a531d 100644
--- a/include/uapi/linux/inotify.h
+++ b/include/uapi/linux/inotify.h
@@ -71,5 +71,13 @@ struct inotify_event {
 #define IN_CLOEXEC O_CLOEXEC
 #define IN_NONBLOCK O_NONBLOCK
 
+/*
+ * ioctl numbers: inotify uses 'I' prefix for all ioctls,
+ * except historical FIONREAD, which is based on 'T'.
+ *
+ * INOTIFY_IOC_SETNEXTWD: set desired number of next created
+ * watch descriptor.
+ */
+#define INOTIFY_IOC_SETNEXTWD	_IOW('I', 0, __s32)
 
 #endif /* _UAPI_LINUX_INOTIFY_H */

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Matthew Wilcox]

On Fri, Feb 09, 2018 at 06:04:54PM +0300, Kirill Tkhai wrote:
> CC: Jan Kara <jack@suse.cz>
> CC: Matthew Wilcox <willy@infradead.org>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: Amir Goldstein <amir73il@gmail.com>
> Signed-off-by: Kirill Tkhai <ktkhai@virtuozzo.com>
> Reviewed-by: Cyrill Gorcunov <gorcunov@openvz.org>

Reviewed-by: Matthew Wilcox <mawilcox@microsoft.com>

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Andrew Morton]

On Fri, 9 Feb 2018 18:04:54 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> wrote:

> Watch descriptor is id of the watch created by inotify_add_watch().
> It is allocated in inotify_add_to_idr(), and takes the numbers
> starting from 1. Every new inotify watch obtains next available
> number (usually, old + 1), as served by idr_alloc_cyclic().
> 
> CRIU (Checkpoint/Restore In Userspace) project supports inotify
> files, and restores watched descriptors with the same numbers,
> they had before dump. Since there was no kernel support, we
> had to use cycle to add a watch with specific descriptor id:
> 
> 	while (1) {
> 		int wd;
> 
> 		wd = inotify_add_watch(inotify_fd, path, mask);
> 		if (wd < 0) {
> 			break;
> 		} else if (wd == desired_wd_id) {
> 			ret = 0;
> 			break;
> 		}
> 
> 		inotify_rm_watch(inotify_fd, wd);
> 	}
> 
> (You may find the actual code at the below link:
>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
> 
> The cycle is suboptiomal and very expensive, but since there is no better
> kernel support, it was the only way to restore that. Happily, we had met
> mostly descriptors with small id, and this approach had worked somehow.
> 
> But recent time containers with inotify with big watch descriptors
> begun to come, and this way stopped to work at all. When descriptor id
> is something about 0x34d71d6, the restoring process spins in busy loop
> for a long time, and the restore hungs and delay of migration from node
> to node could easily be watched.
> 
> This patch aims to solve this problem. It introduces new ioctl
> INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
> watch descriptor from userspace. It simply calls idr_set_cursor() primitive
> to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
> will return this id, if it is not occupied. This is the way which is
> used to restore some other resources from userspace. For example,
> /proc/sys/kernel/ns_last_pid works the same for task pids.
> 
> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
> may exclude it.
> 

Reviewed-by: Andrew Morton <akpm@linux-foundation.org>

With a little cleanup:

--- a/fs/notify/inotify/inotify_user.c~inotify-extend-ioctl-to-allow-to-request-id-of-new-watch-descriptor-fix
+++ a/fs/notify/inotify/inotify_user.c
@@ -285,7 +285,6 @@ static int inotify_release(struct inode
 static long inotify_ioctl(struct file *file, unsigned int cmd,
 			  unsigned long arg)
 {
-	struct inotify_group_private_data *data __maybe_unused;
 	struct fsnotify_group *group;
 	struct fsnotify_event *fsn_event;
 	void __user *p;
@@ -294,7 +293,6 @@ static long inotify_ioctl(struct file *f
 
 	group = file->private_data;
 	p = (void __user *) arg;
-	data = &group->inotify_data;
 
 	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
 
@@ -313,6 +311,9 @@ static long inotify_ioctl(struct file *f
 	case INOTIFY_IOC_SETNEXTWD:
 		ret = -EINVAL;
 		if (arg >= 1 && arg <= INT_MAX) {
+			struct inotify_group_private_data *data;
+
+			data = &group->inotify_data;
 			spin_lock(&data->idr_lock);
 			idr_set_cursor(&data->idr, (unsigned int)arg);
 			spin_unlock(&data->idr_lock);
_

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

On 09.02.2018 23:56, Andrew Morton wrote:
> On Fri, 9 Feb 2018 18:04:54 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> 
>> Watch descriptor is id of the watch created by inotify_add_watch().
>> It is allocated in inotify_add_to_idr(), and takes the numbers
>> starting from 1. Every new inotify watch obtains next available
>> number (usually, old + 1), as served by idr_alloc_cyclic().
>>
>> CRIU (Checkpoint/Restore In Userspace) project supports inotify
>> files, and restores watched descriptors with the same numbers,
>> they had before dump. Since there was no kernel support, we
>> had to use cycle to add a watch with specific descriptor id:
>>
>> 	while (1) {
>> 		int wd;
>>
>> 		wd = inotify_add_watch(inotify_fd, path, mask);
>> 		if (wd < 0) {
>> 			break;
>> 		} else if (wd == desired_wd_id) {
>> 			ret = 0;
>> 			break;
>> 		}
>>
>> 		inotify_rm_watch(inotify_fd, wd);
>> 	}
>>
>> (You may find the actual code at the below link:
>>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
>>
>> The cycle is suboptiomal and very expensive, but since there is no better
>> kernel support, it was the only way to restore that. Happily, we had met
>> mostly descriptors with small id, and this approach had worked somehow.
>>
>> But recent time containers with inotify with big watch descriptors
>> begun to come, and this way stopped to work at all. When descriptor id
>> is something about 0x34d71d6, the restoring process spins in busy loop
>> for a long time, and the restore hungs and delay of migration from node
>> to node could easily be watched.
>>
>> This patch aims to solve this problem. It introduces new ioctl
>> INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
>> watch descriptor from userspace. It simply calls idr_set_cursor() primitive
>> to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
>> will return this id, if it is not occupied. This is the way which is
>> used to restore some other resources from userspace. For example,
>> /proc/sys/kernel/ns_last_pid works the same for task pids.
>>
>> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
>> may exclude it.
>>
> 
> Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> 
> With a little cleanup:
> 
> --- a/fs/notify/inotify/inotify_user.c~inotify-extend-ioctl-to-allow-to-request-id-of-new-watch-descriptor-fix
> +++ a/fs/notify/inotify/inotify_user.c
> @@ -285,7 +285,6 @@ static int inotify_release(struct inode
>  static long inotify_ioctl(struct file *file, unsigned int cmd,
>  			  unsigned long arg)
>  {
> -	struct inotify_group_private_data *data __maybe_unused;
>  	struct fsnotify_group *group;
>  	struct fsnotify_event *fsn_event;
>  	void __user *p;
> @@ -294,7 +293,6 @@ static long inotify_ioctl(struct file *f
>  
>  	group = file->private_data;
>  	p = (void __user *) arg;
> -	data = &group->inotify_data;
>  
>  	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
>  
> @@ -313,6 +311,9 @@ static long inotify_ioctl(struct file *f
>  	case INOTIFY_IOC_SETNEXTWD:
>  		ret = -EINVAL;
>  		if (arg >= 1 && arg <= INT_MAX) {
> +			struct inotify_group_private_data *data;
> +
> +			data = &group->inotify_data;
>  			spin_lock(&data->idr_lock);
>  			idr_set_cursor(&data->idr, (unsigned int)arg);
>  			spin_unlock(&data->idr_lock);

I have no objections.

Thanks,
Kirill

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Stef Bon]

2018-02-09 23:45 GMT+01:00 Kirill Tkhai <ktkhai@virtuozzo.com>:
> On 09.02.2018 23:56, Andrew Morton wrote:
>> On Fri, 9 Feb 2018 18:04:54 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>
>>> Watch descriptor is id of the watch created by inotify_add_watch().
>>> It is allocated in inotify_add_to_idr(), and takes the numbers
>>> starting from 1. Every new inotify watch obtains next available
>>> number (usually, old + 1), as served by idr_alloc_cyclic().
>>>
>>> CRIU (Checkpoint/Restore In Userspace) project supports inotify
>>> files, and restores watched descriptors with the same numbers,
>>> they had before dump. Since there was no kernel support, we
>>> had to use cycle to add a watch with specific descriptor id:
>>>
>>>      while (1) {
>>>              int wd;
>>>
>>>              wd = inotify_add_watch(inotify_fd, path, mask);
>>>              if (wd < 0) {
>>>                      break;
>>>              } else if (wd == desired_wd_id) {
>>>                      ret = 0;
>>>                      break;
>>>              }
>>>
>>>              inotify_rm_watch(inotify_fd, wd);
>>>      }
>>>
>>> (You may find the actual code at the below link:
>>>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)

Well using a ioctl command to force a specific wd is possible, but
isn't it also possible
to do a "freeze" of all (inotify) watches which are involved, and
"unfreeze" when restoring?

Stef

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

On 11.02.2018 14:30, Stef Bon wrote:
> 2018-02-09 23:45 GMT+01:00 Kirill Tkhai <ktkhai@virtuozzo.com>:
>> On 09.02.2018 23:56, Andrew Morton wrote:
>>> On Fri, 9 Feb 2018 18:04:54 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
>>>
>>>> Watch descriptor is id of the watch created by inotify_add_watch().
>>>> It is allocated in inotify_add_to_idr(), and takes the numbers
>>>> starting from 1. Every new inotify watch obtains next available
>>>> number (usually, old + 1), as served by idr_alloc_cyclic().
>>>>
>>>> CRIU (Checkpoint/Restore In Userspace) project supports inotify
>>>> files, and restores watched descriptors with the same numbers,
>>>> they had before dump. Since there was no kernel support, we
>>>> had to use cycle to add a watch with specific descriptor id:
>>>>
>>>>      while (1) {
>>>>              int wd;
>>>>
>>>>              wd = inotify_add_watch(inotify_fd, path, mask);
>>>>              if (wd < 0) {
>>>>                      break;
>>>>              } else if (wd == desired_wd_id) {
>>>>                      ret = 0;
>>>>                      break;
>>>>              }
>>>>
>>>>              inotify_rm_watch(inotify_fd, wd);
>>>>      }
>>>>
>>>> (You may find the actual code at the below link:
>>>>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
> 
> Well using a ioctl command to force a specific wd is possible, but
> isn't it also possible
> to do a "freeze" of all (inotify) watches which are involved, and
> "unfreeze" when restoring?

Regarding C/R, all inotifies are involved ;) Also, all regular files, sockets,
memory mappings, etc. 

Checkpoint code attaches to a process via ptrace() and injects parasite code,
which collects data and metadata of all the process's entities. It's rather
difficult action, because several processes may be checkpointed, and they may
share files/memory mappings/fs/etc. Also, they may be related to different
namespaces. It's long to tell. You may dive into CRIU code, if you're interested.

Then, restore code tries to recreate the processes from ground (possible,
on another physical machine). It uses standard linux system calls to do that,
i.e., it starts from clone() and then creates everything else. When there is
the time to restore a file (inotify in our case), standard linux inotify_init1()
is called. We create the inotify fd, then dup2() it to appropriate number.
Then, we need to add watched files/directories to the inotify. And they must
be added with the same watch descriptor id, as they was at checkpoint time.
We use inotify_add_watch() and it returns id == 1, as you can see in kernel
code. But we need another id, say, 0xfffff. And there is no syscall like dup2()
for inotify watch descriptors. So, we use cyclic inotify_add_watch()/inotify_rm_watch()
as next inotify_add_watch() returns incremented id (see the kernel) despite inotify_rm_watch()
was called to remove old. After 0xfffff-1 iterations, inotify_add_watch() reaches
id we need and returns it. This scheme is very slow, and the patch allows to
restory inotify using 2 syscalls only (ioctl+inotify_add_watch).

So, answering your question: No, it's not possible to use freeze/unfreeze
to do that.

Kirill

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Kirill Tkhai]

On 11.02.2018 14:30, Stef Bon wrote:
> 2018-02-09 23:45 GMT+01:00 Kirill Tkhai <ktkhai-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org>:
>> On 09.02.2018 23:56, Andrew Morton wrote:
>>> On Fri, 9 Feb 2018 18:04:54 +0300 Kirill Tkhai <ktkhai-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org> wrote:
>>>
>>>> Watch descriptor is id of the watch created by inotify_add_watch().
>>>> It is allocated in inotify_add_to_idr(), and takes the numbers
>>>> starting from 1. Every new inotify watch obtains next available
>>>> number (usually, old + 1), as served by idr_alloc_cyclic().
>>>>
>>>> CRIU (Checkpoint/Restore In Userspace) project supports inotify
>>>> files, and restores watched descriptors with the same numbers,
>>>> they had before dump. Since there was no kernel support, we
>>>> had to use cycle to add a watch with specific descriptor id:
>>>>
>>>>      while (1) {
>>>>              int wd;
>>>>
>>>>              wd = inotify_add_watch(inotify_fd, path, mask);
>>>>              if (wd < 0) {
>>>>                      break;
>>>>              } else if (wd == desired_wd_id) {
>>>>                      ret = 0;
>>>>                      break;
>>>>              }
>>>>
>>>>              inotify_rm_watch(inotify_fd, wd);
>>>>      }
>>>>
>>>> (You may find the actual code at the below link:
>>>>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
> 
> Well using a ioctl command to force a specific wd is possible, but
> isn't it also possible
> to do a "freeze" of all (inotify) watches which are involved, and
> "unfreeze" when restoring?

Regarding C/R, all inotifies are involved ;) Also, all regular files, sockets,
memory mappings, etc. 

Checkpoint code attaches to a process via ptrace() and injects parasite code,
which collects data and metadata of all the process's entities. It's rather
difficult action, because several processes may be checkpointed, and they may
share files/memory mappings/fs/etc. Also, they may be related to different
namespaces. It's long to tell. You may dive into CRIU code, if you're interested.

Then, restore code tries to recreate the processes from ground (possible,
on another physical machine). It uses standard linux system calls to do that,
i.e., it starts from clone() and then creates everything else. When there is
the time to restore a file (inotify in our case), standard linux inotify_init1()
is called. We create the inotify fd, then dup2() it to appropriate number.
Then, we need to add watched files/directories to the inotify. And they must
be added with the same watch descriptor id, as they was at checkpoint time.
We use inotify_add_watch() and it returns id == 1, as you can see in kernel
code. But we need another id, say, 0xfffff. And there is no syscall like dup2()
for inotify watch descriptors. So, we use cyclic inotify_add_watch()/inotify_rm_watch()
as next inotify_add_watch() returns incremented id (see the kernel) despite inotify_rm_watch()
was called to remove old. After 0xfffff-1 iterations, inotify_add_watch() reaches
id we need and returns it. This scheme is very slow, and the patch allows to
restory inotify using 2 syscalls only (ioctl+inotify_add_watch).

So, answering your question: No, it's not possible to use freeze/unfreeze
to do that.

Kirill

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Jan Kara]

On Sat 10-02-18 01:45:16, Kirill Tkhai wrote:
> On 09.02.2018 23:56, Andrew Morton wrote:
> > On Fri, 9 Feb 2018 18:04:54 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> wrote:
> > 
> >> Watch descriptor is id of the watch created by inotify_add_watch().
> >> It is allocated in inotify_add_to_idr(), and takes the numbers
> >> starting from 1. Every new inotify watch obtains next available
> >> number (usually, old + 1), as served by idr_alloc_cyclic().
> >>
> >> CRIU (Checkpoint/Restore In Userspace) project supports inotify
> >> files, and restores watched descriptors with the same numbers,
> >> they had before dump. Since there was no kernel support, we
> >> had to use cycle to add a watch with specific descriptor id:
> >>
> >> 	while (1) {
> >> 		int wd;
> >>
> >> 		wd = inotify_add_watch(inotify_fd, path, mask);
> >> 		if (wd < 0) {
> >> 			break;
> >> 		} else if (wd == desired_wd_id) {
> >> 			ret = 0;
> >> 			break;
> >> 		}
> >>
> >> 		inotify_rm_watch(inotify_fd, wd);
> >> 	}
> >>
> >> (You may find the actual code at the below link:
> >>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
> >>
> >> The cycle is suboptiomal and very expensive, but since there is no better
> >> kernel support, it was the only way to restore that. Happily, we had met
> >> mostly descriptors with small id, and this approach had worked somehow.
> >>
> >> But recent time containers with inotify with big watch descriptors
> >> begun to come, and this way stopped to work at all. When descriptor id
> >> is something about 0x34d71d6, the restoring process spins in busy loop
> >> for a long time, and the restore hungs and delay of migration from node
> >> to node could easily be watched.
> >>
> >> This patch aims to solve this problem. It introduces new ioctl
> >> INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
> >> watch descriptor from userspace. It simply calls idr_set_cursor() primitive
> >> to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
> >> will return this id, if it is not occupied. This is the way which is
> >> used to restore some other resources from userspace. For example,
> >> /proc/sys/kernel/ns_last_pid works the same for task pids.
> >>
> >> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
> >> may exclude it.
> >>
> > 
> > Reviewed-by: Andrew Morton <akpm@linux-foundation.org>
> > 
> > With a little cleanup:
> > 
> > --- a/fs/notify/inotify/inotify_user.c~inotify-extend-ioctl-to-allow-to-request-id-of-new-watch-descriptor-fix
> > +++ a/fs/notify/inotify/inotify_user.c
> > @@ -285,7 +285,6 @@ static int inotify_release(struct inode
> >  static long inotify_ioctl(struct file *file, unsigned int cmd,
> >  			  unsigned long arg)
> >  {
> > -	struct inotify_group_private_data *data __maybe_unused;
> >  	struct fsnotify_group *group;
> >  	struct fsnotify_event *fsn_event;
> >  	void __user *p;
> > @@ -294,7 +293,6 @@ static long inotify_ioctl(struct file *f
> >  
> >  	group = file->private_data;
> >  	p = (void __user *) arg;
> > -	data = &group->inotify_data;
> >  
> >  	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
> >  
> > @@ -313,6 +311,9 @@ static long inotify_ioctl(struct file *f
> >  	case INOTIFY_IOC_SETNEXTWD:
> >  		ret = -EINVAL;
> >  		if (arg >= 1 && arg <= INT_MAX) {
> > +			struct inotify_group_private_data *data;
> > +
> > +			data = &group->inotify_data;
> >  			spin_lock(&data->idr_lock);
> >  			idr_set_cursor(&data->idr, (unsigned int)arg);
> >  			spin_unlock(&data->idr_lock);
> 
> I have no objections.

Thanks guys, I have added the patch (with cleanup included) to my tree.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] inotify: Extend ioctl to allow to request id of new watch descriptor

[Jan Kara]

On Sat 10-02-18 01:45:16, Kirill Tkhai wrote:
> On 09.02.2018 23:56, Andrew Morton wrote:
> > On Fri, 9 Feb 2018 18:04:54 +0300 Kirill Tkhai <ktkhai-5HdwGun5lf+gSpxsJD1C4w@public.gmane.org> wrote:
> > 
> >> Watch descriptor is id of the watch created by inotify_add_watch().
> >> It is allocated in inotify_add_to_idr(), and takes the numbers
> >> starting from 1. Every new inotify watch obtains next available
> >> number (usually, old + 1), as served by idr_alloc_cyclic().
> >>
> >> CRIU (Checkpoint/Restore In Userspace) project supports inotify
> >> files, and restores watched descriptors with the same numbers,
> >> they had before dump. Since there was no kernel support, we
> >> had to use cycle to add a watch with specific descriptor id:
> >>
> >> 	while (1) {
> >> 		int wd;
> >>
> >> 		wd = inotify_add_watch(inotify_fd, path, mask);
> >> 		if (wd < 0) {
> >> 			break;
> >> 		} else if (wd == desired_wd_id) {
> >> 			ret = 0;
> >> 			break;
> >> 		}
> >>
> >> 		inotify_rm_watch(inotify_fd, wd);
> >> 	}
> >>
> >> (You may find the actual code at the below link:
> >>  https://github.com/checkpoint-restore/criu/blob/v3.7/criu/fsnotify.c#L577)
> >>
> >> The cycle is suboptiomal and very expensive, but since there is no better
> >> kernel support, it was the only way to restore that. Happily, we had met
> >> mostly descriptors with small id, and this approach had worked somehow.
> >>
> >> But recent time containers with inotify with big watch descriptors
> >> begun to come, and this way stopped to work at all. When descriptor id
> >> is something about 0x34d71d6, the restoring process spins in busy loop
> >> for a long time, and the restore hungs and delay of migration from node
> >> to node could easily be watched.
> >>
> >> This patch aims to solve this problem. It introduces new ioctl
> >> INOTIFY_IOC_SETNEXTWD, which allows to request the number of next created
> >> watch descriptor from userspace. It simply calls idr_set_cursor() primitive
> >> to populate idr::idr_next, so that next idr_alloc_cyclic() allocation
> >> will return this id, if it is not occupied. This is the way which is
> >> used to restore some other resources from userspace. For example,
> >> /proc/sys/kernel/ns_last_pid works the same for task pids.
> >>
> >> The new code is under CONFIG_CHECKPOINT_RESTORE #define, so small system
> >> may exclude it.
> >>
> > 
> > Reviewed-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
> > 
> > With a little cleanup:
> > 
> > --- a/fs/notify/inotify/inotify_user.c~inotify-extend-ioctl-to-allow-to-request-id-of-new-watch-descriptor-fix
> > +++ a/fs/notify/inotify/inotify_user.c
> > @@ -285,7 +285,6 @@ static int inotify_release(struct inode
> >  static long inotify_ioctl(struct file *file, unsigned int cmd,
> >  			  unsigned long arg)
> >  {
> > -	struct inotify_group_private_data *data __maybe_unused;
> >  	struct fsnotify_group *group;
> >  	struct fsnotify_event *fsn_event;
> >  	void __user *p;
> > @@ -294,7 +293,6 @@ static long inotify_ioctl(struct file *f
> >  
> >  	group = file->private_data;
> >  	p = (void __user *) arg;
> > -	data = &group->inotify_data;
> >  
> >  	pr_debug("%s: group=%p cmd=%u\n", __func__, group, cmd);
> >  
> > @@ -313,6 +311,9 @@ static long inotify_ioctl(struct file *f
> >  	case INOTIFY_IOC_SETNEXTWD:
> >  		ret = -EINVAL;
> >  		if (arg >= 1 && arg <= INT_MAX) {
> > +			struct inotify_group_private_data *data;
> > +
> > +			data = &group->inotify_data;
> >  			spin_lock(&data->idr_lock);
> >  			idr_set_cursor(&data->idr, (unsigned int)arg);
> >  			spin_unlock(&data->idr_lock);
> 
> I have no objections.

Thanks guys, I have added the patch (with cleanup included) to my tree.

								Honza
-- 
Jan Kara <jack-IBi9RG/b67k@public.gmane.org>
SUSE Labs, CR