[PATCH 05/26] fs: Convert unnamed_dev_ida to new API

[PATCH 05/26] fs: Convert unnamed_dev_ida to new API

[Matthew Wilcox]

The new API is much easier for this user.  Also add kerneldoc for
get_anon_bdev().

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 fs/super.c | 63 +++++++++++++++++++-----------------------------------
 1 file changed, 22 insertions(+), 41 deletions(-)

diff --git a/fs/super.c b/fs/super.c
index 50728d9c1a05..3e7a0aea716a 100644
--- a/fs/super.c
+++ b/fs/super.c
@@ -978,58 +978,42 @@ void emergency_thaw_all(void)
 	}
 }
 
-/*
- * Unnamed block devices are dummy devices used by virtual
- * filesystems which don't use real block-devices.  -- jrs
- */
-
 static DEFINE_IDA(unnamed_dev_ida);
-static DEFINE_SPINLOCK(unnamed_dev_lock);/* protects the above */
-/* Many userspace utilities consider an FSID of 0 invalid.
- * Always return at least 1 from get_anon_bdev.
- */
-static int unnamed_dev_start = 1;
 
+/**
+ * get_anon_bdev - Allocate a block device for filesystems which don't have one.
+ * @p: Pointer to a dev_t.
+ *
+ * Filesystems which don't use real block devices can call this function
+ * to allocate a virtual block device.
+ *
+ * Context: Any context.  Frequently called while holding sb_lock.
+ * Return: 0 on success, -EMFILE if there are no anonymous bdevs left
+ * or -EAGAIN if memory allocation failed.
+ */
 int get_anon_bdev(dev_t *p)
 {
 	int dev;
-	int error;
 
- retry:
-	if (ida_pre_get(&unnamed_dev_ida, GFP_ATOMIC) == 0)
-		return -ENOMEM;
-	spin_lock(&unnamed_dev_lock);
-	error = ida_get_new_above(&unnamed_dev_ida, unnamed_dev_start, &dev);
-	if (!error)
-		unnamed_dev_start = dev + 1;
-	spin_unlock(&unnamed_dev_lock);
-	if (error == -EAGAIN)
-		/* We raced and lost with another CPU. */
-		goto retry;
-	else if (error)
+	/*
+	 * Many userspace utilities consider an FSID of 0 invalid.
+	 * Always return at least 1 from get_anon_bdev.
+	 */
+	dev = ida_alloc_range(&unnamed_dev_ida, 1, (1 << MINORBITS) - 1,
+			GFP_ATOMIC);
+	if (dev == -ENOSPC)
+		return -EMFILE;
+	if (dev < 0)
 		return -EAGAIN;
 
-	if (dev >= (1 << MINORBITS)) {
-		spin_lock(&unnamed_dev_lock);
-		ida_remove(&unnamed_dev_ida, dev);
-		if (unnamed_dev_start > dev)
-			unnamed_dev_start = dev;
-		spin_unlock(&unnamed_dev_lock);
-		return -EMFILE;
-	}
-	*p = MKDEV(0, dev & MINORMASK);
+	*p = MKDEV(0, dev);
 	return 0;
 }
 EXPORT_SYMBOL(get_anon_bdev);
 
 void free_anon_bdev(dev_t dev)
 {
-	int slot = MINOR(dev);
-	spin_lock(&unnamed_dev_lock);
-	ida_remove(&unnamed_dev_ida, slot);
-	if (slot < unnamed_dev_start)
-		unnamed_dev_start = slot;
-	spin_unlock(&unnamed_dev_lock);
+	ida_free(&unnamed_dev_ida, MINOR(dev));
 }
 EXPORT_SYMBOL(free_anon_bdev);
 
@@ -1037,7 +1021,6 @@ int set_anon_super(struct super_block *s, void *data)
 {
 	return get_anon_bdev(&s->s_dev);
 }
-
 EXPORT_SYMBOL(set_anon_super);
 
 void kill_anon_super(struct super_block *sb)
@@ -1046,7 +1029,6 @@ void kill_anon_super(struct super_block *sb)
 	generic_shutdown_super(sb);
 	free_anon_bdev(dev);
 }
-
 EXPORT_SYMBOL(kill_anon_super);
 
 void kill_litter_super(struct super_block *sb)
@@ -1055,7 +1037,6 @@ void kill_litter_super(struct super_block *sb)
 		d_genocide(sb->s_root);
 	kill_anon_super(sb);
 }
-
 EXPORT_SYMBOL(kill_litter_super);
 
 static int ns_test_super(struct super_block *sb, void *data)
-- 
2.17.1

[PATCH 06/26] fs: Convert namespace IDAs to new API

[Matthew Wilcox]

We don't need to keep track of the starting value; the IDA is efficient.

Signed-off-by: Matthew Wilcox <willy@infradead.org>
---
 fs/namespace.c | 50 ++++++++++++--------------------------------------
 1 file changed, 12 insertions(+), 38 deletions(-)

diff --git a/fs/namespace.c b/fs/namespace.c
index 8ddd14806799..bfd33eaab5aa 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -61,9 +61,6 @@ __setup("mphash_entries=", set_mphash_entries);
 static u64 event;
 static DEFINE_IDA(mnt_id_ida);
 static DEFINE_IDA(mnt_group_ida);
-static DEFINE_SPINLOCK(mnt_id_lock);
-static int mnt_id_start = 0;
-static int mnt_group_start = 1;
 
 static struct hlist_head *mount_hashtable __read_mostly;
 static struct hlist_head *mountpoint_hashtable __read_mostly;
@@ -101,50 +98,30 @@ static inline struct hlist_head *mp_hash(struct dentry *dentry)
 
 static int mnt_alloc_id(struct mount *mnt)
 {
-	int res;
+	int res = ida_alloc(&mnt_id_ida, GFP_KERNEL);
 
-retry:
-	ida_pre_get(&mnt_id_ida, GFP_KERNEL);
-	spin_lock(&mnt_id_lock);
-	res = ida_get_new_above(&mnt_id_ida, mnt_id_start, &mnt->mnt_id);
-	if (!res)
-		mnt_id_start = mnt->mnt_id + 1;
-	spin_unlock(&mnt_id_lock);
-	if (res == -EAGAIN)
-		goto retry;
-
-	return res;
+	if (res < 0)
+		return res;
+	mnt->mnt_id = res;
+	return 0;
 }
 
 static void mnt_free_id(struct mount *mnt)
 {
-	int id = mnt->mnt_id;
-	spin_lock(&mnt_id_lock);
-	ida_remove(&mnt_id_ida, id);
-	if (mnt_id_start > id)
-		mnt_id_start = id;
-	spin_unlock(&mnt_id_lock);
+	ida_free(&mnt_id_ida, mnt->mnt_id);
 }
 
 /*
  * Allocate a new peer group ID
- *
- * mnt_group_ida is protected by namespace_sem
  */
 static int mnt_alloc_group_id(struct mount *mnt)
 {
-	int res;
+	int res = ida_alloc_min(&mnt_group_ida, 1, GFP_KERNEL);
 
-	if (!ida_pre_get(&mnt_group_ida, GFP_KERNEL))
-		return -ENOMEM;
-
-	res = ida_get_new_above(&mnt_group_ida,
-				mnt_group_start,
-				&mnt->mnt_group_id);
-	if (!res)
-		mnt_group_start = mnt->mnt_group_id + 1;
-
-	return res;
+	if (res < 0)
+		return res;
+	mnt->mnt_group_id = res;
+	return 0;
 }
 
 /*
@@ -152,10 +129,7 @@ static int mnt_alloc_group_id(struct mount *mnt)
  */
 void mnt_release_group_id(struct mount *mnt)
 {
-	int id = mnt->mnt_group_id;
-	ida_remove(&mnt_group_ida, id);
-	if (mnt_group_start > id)
-		mnt_group_start = id;
+	ida_free(&mnt_group_ida, mnt->mnt_group_id);
 	mnt->mnt_group_id = 0;
 }
 
-- 
2.17.1

Re: [PATCH 05/26] fs: Convert unnamed_dev_ida to new API

[Randy Dunlap]

On 06/21/2018 02:28 PM, Matthew Wilcox wrote:
> The new API is much easier for this user.  Also add kerneldoc for
> get_anon_bdev().
> 
> Signed-off-by: Matthew Wilcox <willy@infradead.org>
> ---
>  fs/super.c | 63 +++++++++++++++++++-----------------------------------
>  1 file changed, 22 insertions(+), 41 deletions(-)
> 
> diff --git a/fs/super.c b/fs/super.c
> index 50728d9c1a05..3e7a0aea716a 100644
> --- a/fs/super.c
> +++ b/fs/super.c
> @@ -978,58 +978,42 @@ void emergency_thaw_all(void)
>  	}
>  }
>  
> -/*
> - * Unnamed block devices are dummy devices used by virtual
> - * filesystems which don't use real block-devices.  -- jrs
> - */
> -
>  static DEFINE_IDA(unnamed_dev_ida);
> -static DEFINE_SPINLOCK(unnamed_dev_lock);/* protects the above */
> -/* Many userspace utilities consider an FSID of 0 invalid.
> - * Always return at least 1 from get_anon_bdev.
> - */
> -static int unnamed_dev_start = 1;
>  
> +/**
> + * get_anon_bdev - Allocate a block device for filesystems which don't have one.
> + * @p: Pointer to a dev_t.
> + *
> + * Filesystems which don't use real block devices can call this function
> + * to allocate a virtual block device.
> + *
> + * Context: Any context.  Frequently called while holding sb_lock.
> + * Return: 0 on success, -EMFILE if there are no anonymous bdevs left
> + * or -EAGAIN if memory allocation failed.

Looks to me like the code used to return -ENOMEM and used -EAGAIN as an
internal retry code.

confused?  (/me)

> + */
>  int get_anon_bdev(dev_t *p)
>  {
>  	int dev;
> -	int error;
>  
> - retry:
> -	if (ida_pre_get(&unnamed_dev_ida, GFP_ATOMIC) == 0)
> -		return -ENOMEM;
> -	spin_lock(&unnamed_dev_lock);
> -	error = ida_get_new_above(&unnamed_dev_ida, unnamed_dev_start, &dev);
> -	if (!error)
> -		unnamed_dev_start = dev + 1;
> -	spin_unlock(&unnamed_dev_lock);
> -	if (error == -EAGAIN)
> -		/* We raced and lost with another CPU. */
> -		goto retry;
> -	else if (error)
> +	/*
> +	 * Many userspace utilities consider an FSID of 0 invalid.
> +	 * Always return at least 1 from get_anon_bdev.
> +	 */
> +	dev = ida_alloc_range(&unnamed_dev_ida, 1, (1 << MINORBITS) - 1,
> +			GFP_ATOMIC);
> +	if (dev == -ENOSPC)
> +		return -EMFILE;
> +	if (dev < 0)
>  		return -EAGAIN;
>  
> -	if (dev >= (1 << MINORBITS)) {
> -		spin_lock(&unnamed_dev_lock);
> -		ida_remove(&unnamed_dev_ida, dev);
> -		if (unnamed_dev_start > dev)
> -			unnamed_dev_start = dev;
> -		spin_unlock(&unnamed_dev_lock);
> -		return -EMFILE;
> -	}
> -	*p = MKDEV(0, dev & MINORMASK);
> +	*p = MKDEV(0, dev);
>  	return 0;
>  }
>  EXPORT_SYMBOL(get_anon_bdev);



-- 
~Randy

Re: [PATCH 05/26] fs: Convert unnamed_dev_ida to new API

[Matthew Wilcox]

On Fri, Jun 22, 2018 at 12:45:10PM -0700, Randy Dunlap wrote:
> > + * Context: Any context.  Frequently called while holding sb_lock.
> > + * Return: 0 on success, -EMFILE if there are no anonymous bdevs left
> > + * or -EAGAIN if memory allocation failed.
> 
> Looks to me like the code used to return -ENOMEM and used -EAGAIN as an
> internal retry code.
> 
> confused?  (/me)

Quite right.

+++ b/fs/super.c
@@ -989,7 +989,7 @@ static DEFINE_IDA(unnamed_dev_ida);
  *
  * Context: Any context.  Frequently called while holding sb_lock.
  * Return: 0 on success, -EMFILE if there are no anonymous bdevs left
- * or -EAGAIN if memory allocation failed.
+ * or -ENOMEM if memory allocation failed.
  */
 int get_anon_bdev(dev_t *p)
 {
@@ -1002,9 +1002,9 @@ int get_anon_bdev(dev_t *p)
        dev = ida_alloc_range(&unnamed_dev_ida, 1, (1 << MINORBITS) - 1,
                        GFP_ATOMIC);
        if (dev == -ENOSPC)
-               return -EMFILE;
+               dev = -EMFILE;
        if (dev < 0)
-               return -EAGAIN;
+               return dev;
 
        *p = MKDEV(0, dev);
        return 0;