From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from bombadil.infradead.org ([65.50.211.133]:51123 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750997AbeBIN2K (ORCPT ); Fri, 9 Feb 2018 08:28:10 -0500 Date: Fri, 9 Feb 2018 05:28:06 -0800 From: Matthew Wilcox To: Kirill Tkhai Cc: akpm@linux-foundation.org, jack@suse.cz, amir73il@gmail.com, linux-fsdevel@vger.kernel.org, gorcunov@virtuozzo.com Subject: Re: [PATCH] inotify: Extend ioctl to allow to request id of new watch descriptor Message-ID: <20180209132806.GA16666@bombadil.infradead.org> References: <151810242614.30935.12876744458891870220.stgit@localhost.localdomain> <20180208160228.GE15846@bombadil.infradead.org> <041af224-8e9b-1b62-fd99-7ffec367138e@virtuozzo.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <041af224-8e9b-1b62-fd99-7ffec367138e@virtuozzo.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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. */