[PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Mark Wu]

Current index allocation in virtio-blk is based on a monotonically
increasing variable "index". It could cause some confusion about disk
name in the case of hot-plugging disks. And it's impossible to find the
lowest available index by just maintaining a simple index. So it's
changed to use ida to allocate index via referring to the index
allocation in scsi disk.

Signed-off-by: Mark Wu <dwu@redhat.com>
---
 drivers/block/virtio_blk.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..ba734b3 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,10 +8,14 @@
 #include <linux/scatterlist.h>
 #include <linux/string_helpers.h>
 #include <scsi/scsi_cmnd.h>
+#include <linux/idr.h>
 
 #define PART_BITS 4
 
-static int major, index;
+static int major;
+static DEFINE_SPINLOCK(vd_index_lock);
+static DEFINE_IDA(vd_index_ida);
+
 struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
@@ -23,6 +27,7 @@ struct virtio_blk
 
 	/* The disk structure for the kernel. */
 	struct gendisk *disk;
+	u32 index;
 
 	/* Request tracking. */
 	struct list_head reqs;
@@ -343,12 +348,26 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	struct request_queue *q;
 	int err;
 	u64 cap;
-	u32 v, blk_size, sg_elems, opt_io_size;
+	u32 v, blk_size, sg_elems, opt_io_size, index;
 	u16 min_io_size;
 	u8 physical_block_exp, alignment_offset;
 
-	if (index_to_minor(index) >= 1 << MINORBITS)
-		return -ENOSPC;
+	do {
+		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
+			return err;
+
+		spin_lock(&vd_index_lock);
+		err = ida_get_new(&vd_index_ida, &index);
+		spin_unlock(&vd_index_lock);
+	} while (err == -EAGAIN);
+
+	if (err)
+		return err;
+
+	if (index_to_minor(index) >= 1 << MINORBITS) {
+		err =  -ENOSPC;
+		goto out_free_index;
+	}
 
 	/* We need to know how many segments before we allocate. */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -421,7 +440,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	vblk->disk->private_data = vblk;
 	vblk->disk->fops = &virtblk_fops;
 	vblk->disk->driverfs_dev = &vdev->dev;
-	index++;
+	vblk->index = index;
 
 	/* configure queue flush support */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
@@ -516,6 +535,10 @@ out_free_vq:
 	vdev->config->del_vqs(vdev);
 out_free_vblk:
 	kfree(vblk);
+out_free_index:
+	spin_lock(&vd_index_lock);
+	ida_remove(&vd_index_ida, index);
+	spin_unlock(&vd_index_lock);
 out:
 	return err;
 }
@@ -529,6 +552,10 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	/* Nothing should be pending. */
 	BUG_ON(!list_empty(&vblk->reqs));
 
+	spin_lock(&vd_index_lock);
+	ida_remove(&vd_index_ida, vblk->index);
+	spin_unlock(&vd_index_lock);
+
 	/* Stop all the virtqueues. */
 	vdev->config->reset(vdev);
 
-- 
1.7.1

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Mark Wu]

On 06/01/2011 03:24 AM, Mark Wu wrote:
> -	if (index_to_minor(index)>= 1<<  MINORBITS)
> -		return -ENOSPC;
> +	do {
> +		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
> +			return err;
> +
There's a problem in above code: err is not initialized before using, so 
change it to return -1;
+       do {
+               if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
+                       return -1;

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Rusty Russell]

On Wed,  1 Jun 2011 03:24:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> Current index allocation in virtio-blk is based on a monotonically
> increasing variable "index". It could cause some confusion about disk
> name in the case of hot-plugging disks. And it's impossible to find the
> lowest available index by just maintaining a simple index. So it's
> changed to use ida to allocate index via referring to the index
> allocation in scsi disk.
> 
> Signed-off-by: Mark Wu <dwu@redhat.com>

Hi Mark,

   I don't believe that we do disk probes in parallel, so the spinlock
is unnecessary.  Otherwise, this looks good.

Thanks,
Rusty.

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Michael S. Tsirkin]

On Wed, Jun 01, 2011 at 03:24:29AM -0400, Mark Wu wrote:
> Current index allocation in virtio-blk is based on a monotonically
> increasing variable "index". It could cause some confusion about disk
> name in the case of hot-plugging disks. And it's impossible to find the
> lowest available index by just maintaining a simple index. So it's
> changed to use ida to allocate index via referring to the index
> allocation in scsi disk.
> 
> Signed-off-by: Mark Wu <dwu@redhat.com>
> ---
>  drivers/block/virtio_blk.c |   37 ++++++++++++++++++++++++++++++++-----
>  1 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 079c088..ba734b3 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -8,10 +8,14 @@
>  #include <linux/scatterlist.h>
>  #include <linux/string_helpers.h>
>  #include <scsi/scsi_cmnd.h>
> +#include <linux/idr.h>
>  
>  #define PART_BITS 4
>  
> -static int major, index;
> +static int major;
> +static DEFINE_SPINLOCK(vd_index_lock);
> +static DEFINE_IDA(vd_index_ida);
> +
>  struct workqueue_struct *virtblk_wq;
>  
>  struct virtio_blk
> @@ -23,6 +27,7 @@ struct virtio_blk
>  
>  	/* The disk structure for the kernel. */
>  	struct gendisk *disk;
> +	u32 index;
>  
>  	/* Request tracking. */
>  	struct list_head reqs;
> @@ -343,12 +348,26 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	struct request_queue *q;
>  	int err;
>  	u64 cap;
> -	u32 v, blk_size, sg_elems, opt_io_size;
> +	u32 v, blk_size, sg_elems, opt_io_size, index;
>  	u16 min_io_size;
>  	u8 physical_block_exp, alignment_offset;
>  
> -	if (index_to_minor(index) >= 1 << MINORBITS)
> -		return -ENOSPC;
> +	do {
> +		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
> +			return err;
> +
> +		spin_lock(&vd_index_lock);
> +		err = ida_get_new(&vd_index_ida, &index);
> +		spin_unlock(&vd_index_lock);
> +	} while (err == -EAGAIN);
> +
> +	if (err)
> +		return err;
> +
> +	if (index_to_minor(index) >= 1 << MINORBITS) {
> +		err =  -ENOSPC;
> +		goto out_free_index;
> +	}
>  
>  	/* We need to know how many segments before we allocate. */
>  	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
> @@ -421,7 +440,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	vblk->disk->private_data = vblk;
>  	vblk->disk->fops = &virtblk_fops;
>  	vblk->disk->driverfs_dev = &vdev->dev;
> -	index++;
> +	vblk->index = index;
>  
>  	/* configure queue flush support */
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
> @@ -516,6 +535,10 @@ out_free_vq:
>  	vdev->config->del_vqs(vdev);
>  out_free_vblk:
>  	kfree(vblk);
> +out_free_index:
> +	spin_lock(&vd_index_lock);
> +	ida_remove(&vd_index_ida, index);
> +	spin_unlock(&vd_index_lock);
>  out:
>  	return err;
>  }
> @@ -529,6 +552,10 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
>  	/* Nothing should be pending. */
>  	BUG_ON(!list_empty(&vblk->reqs));
>  
> +	spin_lock(&vd_index_lock);
> +	ida_remove(&vd_index_ida, vblk->index);
> +	spin_unlock(&vd_index_lock);
> +
>  	/* Stop all the virtqueues. */
>  	vdev->config->reset(vdev);

As we get index first thing in _probe, let's remove last thing
in _remove.

I'm not sure violating the rule of cleanup
in the reverse order of initialization can lead
to problems here, but it's better to stick to this rule regardless,
IMO.

> -- 
> 1.7.1

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Michael S. Tsirkin]

On Wed, Jun 01, 2011 at 04:25:48AM -0400, Mark Wu wrote:
> On 06/01/2011 03:24 AM, Mark Wu wrote:
> >-	if (index_to_minor(index)>= 1<<  MINORBITS)
> >-		return -ENOSPC;
> >+	do {
> >+		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
> >+			return err;
> >+
> There's a problem in above code: err is not initialized before
> using, so change it to return -1;
> +       do {
> +               if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
> +                       return -1;

Not -1. Pls return -ENOMEM.

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Mark Wu]

On 06/01/2011 07:57 PM, Rusty Russell wrote:
> On Wed,  1 Jun 2011 03:24:29 -0400, Mark Wu <dwu@redhat.com> wrote:
>> Current index allocation in virtio-blk is based on a monotonically
>>  increasing variable "index". It could cause some confusion about 
>> disk name in the case of hot-plugging disks. And it's impossible
>> to find the lowest available index by just maintaining a simple
>> index. So it's changed to use ida to allocate index via referring
>> to the index allocation in scsi disk.
>> 
>> Signed-off-by: Mark Wu <dwu@redhat.com>
> 
> Hi Mark,
> 
> I don't believe that we do disk probes in parallel, so the spinlock 
> is unnecessary.  Otherwise, this looks good.
> 
> Thanks, Rusty.
Hi Rusty,
Yes, I can't figure out an instance of disk probing in parallel either, but as
per the following commit, I think we still need use lock for safety. What's your opinion?

commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
Author: Tejun Heo <tj@kernel.org>
Date:   Sat Feb 21 11:04:45 2009 +0900

    [SCSI] sd: revive sd_index_lock

    Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
    use ida instead of idr incorrectly removed sd_index_lock around id
    allocation and free.  idr/ida do have internal locks but they protect
    their free object lists not the allocation itself.  The caller is
    responsible for that.  This missing synchronization led to the same id
    being assigned to multiple devices leading to oops.

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Mark Wu]

On 06/02/2011 06:34 AM, Michael S. Tsirkin wrote:
> On Wed, Jun 01, 2011 at 04:25:48AM -0400, Mark Wu wrote:
>> On 06/01/2011 03:24 AM, Mark Wu wrote:
>>> -	if (index_to_minor(index)>= 1<<  MINORBITS)
>>> -		return -ENOSPC;
>>> +	do {
>>> +		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
>>> +			return err;
>>> +
>> There's a problem in above code: err is not initialized before
>> using, so change it to return -1;
>> +       do {
>> +               if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
>> +                       return -1;
> 
> Not -1. Pls return -ENOMEM.
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hi Michael,
Thanks for pointing out that. This is the revised patch.


>From ffe49efd20938952a09d5a87fe694a6f62937756 Mon Sep 17 00:00:00 2001
From: Mark Wu <dwu@redhat.com>
Date: Wed, 8 Jun 2011 08:25:53 -0400
Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

Current index allocation in virtio-blk is based on a monotonically
increasing variable "index". It could cause some confusion about disk
name in the case of hot-plugging disks. And it's impossible to find the
lowest available index by just maintaining a simple index. So it's
changed to use ida to allocate index via referring to the index
allocation in scsi disk.

Signed-off-by: Mark Wu <dwu@redhat.com>
---
 drivers/block/virtio_blk.c |   37 ++++++++++++++++++++++++++++++++-----
 1 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..f13b758 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,10 +8,14 @@
 #include <linux/scatterlist.h>
 #include <linux/string_helpers.h>
 #include <scsi/scsi_cmnd.h>
+#include <linux/idr.h>
 
 #define PART_BITS 4
 
-static int major, index;
+static int major;
+static DEFINE_SPINLOCK(vd_index_lock);
+static DEFINE_IDA(vd_index_ida);
+
 struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
@@ -23,6 +27,7 @@ struct virtio_blk
 
 	/* The disk structure for the kernel. */
 	struct gendisk *disk;
+	u32 index;
 
 	/* Request tracking. */
 	struct list_head reqs;
@@ -343,12 +348,26 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	struct request_queue *q;
 	int err;
 	u64 cap;
-	u32 v, blk_size, sg_elems, opt_io_size;
+	u32 v, blk_size, sg_elems, opt_io_size, index;
 	u16 min_io_size;
 	u8 physical_block_exp, alignment_offset;
 
-	if (index_to_minor(index) >= 1 << MINORBITS)
-		return -ENOSPC;
+	do {
+		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
+			return -ENOMEM;
+
+		spin_lock(&vd_index_lock);
+		err = ida_get_new(&vd_index_ida, &index);
+		spin_unlock(&vd_index_lock);
+	} while (err == -EAGAIN);
+
+	if (err)
+		return err;
+
+	if (index_to_minor(index) >= 1 << MINORBITS) {
+		err =  -ENOSPC;
+		goto out_free_index;
+	}
 
 	/* We need to know how many segments before we allocate. */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -421,7 +440,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	vblk->disk->private_data = vblk;
 	vblk->disk->fops = &virtblk_fops;
 	vblk->disk->driverfs_dev = &vdev->dev;
-	index++;
+	vblk->index = index;
 
 	/* configure queue flush support */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
@@ -516,6 +535,10 @@ out_free_vq:
 	vdev->config->del_vqs(vdev);
 out_free_vblk:
 	kfree(vblk);
+out_free_index:
+	spin_lock(&vd_index_lock);
+	ida_remove(&vd_index_ida, index);
+	spin_unlock(&vd_index_lock);
 out:
 	return err;
 }
@@ -538,6 +561,10 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	mempool_destroy(vblk->pool);
 	vdev->config->del_vqs(vdev);
 	kfree(vblk);
+
+	spin_lock(&vd_index_lock);
+	ida_remove(&vd_index_ida, vblk->index);
+	spin_unlock(&vd_index_lock);
 }
 
 static const struct virtio_device_id id_table[] = {
-- 
1.7.1

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Rusty Russell]

On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> Hi Rusty,
> Yes, I can't figure out an instance of disk probing in parallel either, but as
> per the following commit, I think we still need use lock for safety. What's your opinion?
> 
> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> Author: Tejun Heo <tj@kernel.org>
> Date:   Sat Feb 21 11:04:45 2009 +0900
> 
>     [SCSI] sd: revive sd_index_lock
> 
>     Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
>     use ida instead of idr incorrectly removed sd_index_lock around id
>     allocation and free.  idr/ida do have internal locks but they protect
>     their free object lists not the allocation itself.  The caller is
>     responsible for that.  This missing synchronization led to the same id
>     being assigned to multiple devices leading to oops.

I'm confused.  Tejun, Greg, anyone can probes happen in parallel?

If so, I'll have to review all my drivers.

Thanks,
Rusty.

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Greg KH]

On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> > Hi Rusty,
> > Yes, I can't figure out an instance of disk probing in parallel either, but as
> > per the following commit, I think we still need use lock for safety. What's your opinion?
> > 
> > commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> > Author: Tejun Heo <tj@kernel.org>
> > Date:   Sat Feb 21 11:04:45 2009 +0900
> > 
> >     [SCSI] sd: revive sd_index_lock
> > 
> >     Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> >     use ida instead of idr incorrectly removed sd_index_lock around id
> >     allocation and free.  idr/ida do have internal locks but they protect
> >     their free object lists not the allocation itself.  The caller is
> >     responsible for that.  This missing synchronization led to the same id
> >     being assigned to multiple devices leading to oops.
> 
> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
> 
> If so, I'll have to review all my drivers.

I know we've tried it in the past, at the PCI device level, and ran into
some issues, but I don't remember if that code ever made it into the
mainline kernel or not.

greg k-h

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Tejun Heo]

Hello,

On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> > Hi Rusty,
> > Yes, I can't figure out an instance of disk probing in parallel either, but as
> > per the following commit, I think we still need use lock for safety. What's your opinion?
> > 
> > commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> > Author: Tejun Heo <tj@kernel.org>
> > Date:   Sat Feb 21 11:04:45 2009 +0900
> > 
> >     [SCSI] sd: revive sd_index_lock
> > 
> >     Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> >     use ida instead of idr incorrectly removed sd_index_lock around id
> >     allocation and free.  idr/ida do have internal locks but they protect
> >     their free object lists not the allocation itself.  The caller is
> >     responsible for that.  This missing synchronization led to the same id
> >     being assigned to multiple devices leading to oops.
> 
> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
> 
> If so, I'll have to review all my drivers.

Unless async is explicitly used, probe happens sequentially.  IOW, if
there's no async_schedule() call, things won't happen in parallel.
That said, I think it wouldn't be such a bad idea to protect ida with
spinlock regardless unless the probe code explicitly requires
serialization.

Thanks.

-- 
tejun

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Mark Wu]

On 06/09/2011 05:14 AM, Tejun Heo wrote:
> Hello,
> 
> On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
>> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
>>> Hi Rusty,
>>> Yes, I can't figure out an instance of disk probing in parallel either, but as
>>> per the following commit, I think we still need use lock for safety. What's your opinion?
>>>
>>> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
>>> Author: Tejun Heo <tj@kernel.org>
>>> Date:   Sat Feb 21 11:04:45 2009 +0900
>>>
>>>     [SCSI] sd: revive sd_index_lock
>>>
>>>     Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
>>>     use ida instead of idr incorrectly removed sd_index_lock around id
>>>     allocation and free.  idr/ida do have internal locks but they protect
>>>     their free object lists not the allocation itself.  The caller is
>>>     responsible for that.  This missing synchronization led to the same id
>>>     being assigned to multiple devices leading to oops.
>>
>> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
>>
>> If so, I'll have to review all my drivers.
> 
> Unless async is explicitly used, probe happens sequentially.  IOW, if
> there's no async_schedule() call, things won't happen in parallel.
> That said, I think it wouldn't be such a bad idea to protect ida with
> spinlock regardless unless the probe code explicitly requires
> serialization.
> 
> Thanks.
> 
Since virtio blk driver doesn't use async probe, it needn't use spinlock to protect ida.
So remove the lock from patch.

>From fbb396df9dbf8023f1b268be01b43529a3993d57 Mon Sep 17 00:00:00 2001
From: Mark Wu <dwu@redhat.com>
Date: Thu, 9 Jun 2011 06:34:07 -0400
Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

Current index allocation in virtio-blk is based on a monotonically
increasing variable "index". It could cause some confusion about disk
name in the case of hot-plugging disks. And it's impossible to find the
lowest available index by just maintaining a simple index. So it's
changed to use ida to allocate index via referring to the index
allocation in scsi disk.

Signed-off-by: Mark Wu <dwu@redhat.com>
---
 drivers/block/virtio_blk.c |   28 +++++++++++++++++++++++-----
 1 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 079c088..bf81ab6 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -8,10 +8,13 @@
 #include <linux/scatterlist.h>
 #include <linux/string_helpers.h>
 #include <scsi/scsi_cmnd.h>
+#include <linux/idr.h>
 
 #define PART_BITS 4
 
-static int major, index;
+static int major;
+static DEFINE_IDA(vd_index_ida);
+
 struct workqueue_struct *virtblk_wq;
 
 struct virtio_blk
@@ -23,6 +26,7 @@ struct virtio_blk
 
 	/* The disk structure for the kernel. */
 	struct gendisk *disk;
+	u32 index;
 
 	/* Request tracking. */
 	struct list_head reqs;
@@ -343,12 +347,23 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	struct request_queue *q;
 	int err;
 	u64 cap;
-	u32 v, blk_size, sg_elems, opt_io_size;
+	u32 v, blk_size, sg_elems, opt_io_size, index;
 	u16 min_io_size;
 	u8 physical_block_exp, alignment_offset;
 
-	if (index_to_minor(index) >= 1 << MINORBITS)
-		return -ENOSPC;
+	do {
+		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
+			return -ENOMEM;
+		err = ida_get_new(&vd_index_ida, &index);
+	} while (err == -EAGAIN);
+
+	if (err)
+		return err;
+
+	if (index_to_minor(index) >= 1 << MINORBITS) {
+		err =  -ENOSPC;
+		goto out_free_index;
+	}
 
 	/* We need to know how many segments before we allocate. */
 	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
@@ -421,7 +436,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
 	vblk->disk->private_data = vblk;
 	vblk->disk->fops = &virtblk_fops;
 	vblk->disk->driverfs_dev = &vdev->dev;
-	index++;
+	vblk->index = index;
 
 	/* configure queue flush support */
 	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
@@ -516,6 +531,8 @@ out_free_vq:
 	vdev->config->del_vqs(vdev);
 out_free_vblk:
 	kfree(vblk);
+out_free_index:
+	ida_remove(&vd_index_ida, index);
 out:
 	return err;
 }
@@ -538,6 +555,7 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
 	mempool_destroy(vblk->pool);
 	vdev->config->del_vqs(vdev);
 	kfree(vblk);
+	ida_remove(&vd_index_ida, vblk->index);
 }
 
 static const struct virtio_device_id id_table[] = {
-- 
1.7.1

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Rusty Russell]

> Since virtio blk driver doesn't use async probe, it needn't use spinlock to protect ida.
> So remove the lock from patch.

OK, that's fine, but:
> -	if (index_to_minor(index) >= 1 << MINORBITS)
> -		return -ENOSPC;
> +	do {
> +		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
> +			return -ENOMEM;
> +		err = ida_get_new(&vd_index_ida, &index);
> +	} while (err == -EAGAIN);
> +
> +	if (err)
> +		return err;
> +
> +	if (index_to_minor(index) >= 1 << MINORBITS) {
> +		err =  -ENOSPC;
> +		goto out_free_index;
> +	}

Is this *really* how this is supposed to be used?

Tejun, this is your code.  What do you think of something like this?
(untested)

Subject: ida: Simplified functions for id allocation.

The current hyper-optimized functions are overkill if you simply want
to allocate an id for a device.  Create versions which use an internal
lock.

Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>

diff --git a/include/linux/idr.h b/include/linux/idr.h
--- a/include/linux/idr.h
+++ b/include/linux/idr.h
@@ -146,6 +146,9 @@ void ida_remove(struct ida *ida, int id)
 void ida_destroy(struct ida *ida);
 void ida_init(struct ida *ida);
 
+int ida_simple_get(struct ida *ida, int min_id, int max_id);
+void ida_simple_remove(struct ida *ida, int id);
+
 void __init idr_init_cache(void);
 
 #endif /* __IDR_H__ */
diff --git a/lib/idr.c b/lib/idr.c
--- a/lib/idr.c
+++ b/lib/idr.c
@@ -34,8 +34,10 @@
 #include <linux/err.h>
 #include <linux/string.h>
 #include <linux/idr.h>
+#include <linux/mutex.h>
 
 static struct kmem_cache *idr_layer_cache;
+static DEFINE_MUTEX(simple_ida);
 
 static struct idr_layer *get_from_free_list(struct idr *idp)
 {
@@ -926,6 +928,52 @@ void ida_destroy(struct ida *ida)
 EXPORT_SYMBOL(ida_destroy);
 
 /**
+ * ida_simple_get - get a new id.
+ * @ida: the (initialized) ida.
+ * @min_id: the minimum id (inclusive)
+ * @max_id: the maximum id (inclusive)
+ *
+ * Allocates an id in the range min_id <= id <= max_id, or returns -ENOSPC.
+ * On allocation failure, returns -ENOMEM.  This function can sleep.
+ *
+ * Use ida_simple_remove() to get rid of an id.
+ */
+int ida_simple_get(struct ida *ida, int min_id, int max_id)
+{
+	int ret;
+
+	mutex_lock(&simple_ida);
+	if (ida_pre_get(ida, GFP_KERNEL)) {
+		int id;
+		ret = ida_get_new_above(ida, min_id, &id);
+		if (!ret) {
+			if (id > max_id) {
+				ida_remove(ida, id);
+				ret = -ENOSPC;
+			} else
+				ret = id;
+		}
+	} else
+		ret = -ENOMEM;
+	mutex_unlock(&simple_ida);
+	return ret;
+}
+EXPORT_SYMBOL(ida_simple_get);
+
+/**
+ * ida_simple_remove - remove an allocated id.
+ * @ida: the (initialized) ida.
+ * @id: the id returned by ida_simple_get.
+ */
+void ida_simple_remove(struct ida *ida, int id)
+{
+	mutex_lock(&simple_ida);
+	ida_remove(ida, id);
+	mutex_unlock(&simple_ida);
+}
+EXPORT_SYMBOL(ida_simple_remove);
+
+/**
  * ida_init - initialize ida handle
  * @ida:	ida handle
  *

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Tejun Heo]

Hello,

On Wed, Jun 15, 2011 at 02:21:51PM +0930, Rusty Russell wrote:
> > +	if (index_to_minor(index) >= 1 << MINORBITS) {
> > +		err =  -ENOSPC;
> > +		goto out_free_index;
> > +	}
> 
> Is this *really* how this is supposed to be used?
> 
> Tejun, this is your code.  What do you think of something like this?
> (untested)

It's inherited from idr which was designed to have separate
prepare/allocation stages so that allocation can happen inside an
outer spinlock.  It doesn't have too much to do with optimization.
It's mostly to be able to use sleepable context for memory allocation
while allowing atomic id[ra] allocation.

>  /**
> + * ida_simple_get - get a new id.
> + * @ida: the (initialized) ida.
> + * @min_id: the minimum id (inclusive)
> + * @max_id: the maximum id (inclusive)
> + *
> + * Allocates an id in the range min_id <= id <= max_id, or returns -ENOSPC.
> + * On allocation failure, returns -ENOMEM.  This function can sleep.
> + *
> + * Use ida_simple_remove() to get rid of an id.
> + */
> +int ida_simple_get(struct ida *ida, int min_id, int max_id)

Hmmm... new interface different from existing id[ra] style, but yeah
something like the above would have made more sense from the
beginning.  The only thing is that isn't (begin <= range < end) more
conventional form to express ranges?

Thanks.

-- 
tejun

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Rusty Russell]

On Wed, 15 Jun 2011 09:06:38 +0200, Tejun Heo <tj@kernel.org> wrote:
> It's inherited from idr which was designed to have separate
> prepare/allocation stages so that allocation can happen inside an
> outer spinlock.  It doesn't have too much to do with optimization.
> It's mostly to be able to use sleepable context for memory allocation
> while allowing atomic id[ra] allocation.

It might have made sense for a few callers, but as a general mechanism
it stinks.  It's a lot of dancing to avoid GFP_ATOMIC allocations; we'd
be better making idr_get_new() take a gfp_t, and have an idr_pre_alloc()
for those who care.

*Sure* there's a chance of racing and we will need to do an atomic
allocation.  But can anyone justify the current complexity for all
callers?

> > + * ida_simple_get - get a new id.
> > + * @ida: the (initialized) ida.
> > + * @min_id: the minimum id (inclusive)
> > + * @max_id: the maximum id (inclusive)
> > + *
> > + * Allocates an id in the range min_id <= id <= max_id, or returns -ENOSPC.
> > + * On allocation failure, returns -ENOMEM.  This function can sleep.
> > + *
> > + * Use ida_simple_remove() to get rid of an id.
> > + */
> > +int ida_simple_get(struct ida *ida, int min_id, int max_id)
> 
> Hmmm... new interface different from existing id[ra] style, but yeah
> something like the above would have made more sense from the
> beginning.  The only thing is that isn't (begin <= range < end) more
> conventional form to express ranges?

Yes, but how to express an unlimited range then?  I could used unsigned
and 0x80000000, but that seemed crude.

Cheers,
Rusty.

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Tejun Heo]

Hello,

On Thu, Jun 16, 2011 at 09:35:34AM +0930, Rusty Russell wrote:
> On Wed, 15 Jun 2011 09:06:38 +0200, Tejun Heo <tj@kernel.org> wrote:
> > It's inherited from idr which was designed to have separate
> > prepare/allocation stages so that allocation can happen inside an
> > outer spinlock.  It doesn't have too much to do with optimization.
> > It's mostly to be able to use sleepable context for memory allocation
> > while allowing atomic id[ra] allocation.
> 
> It might have made sense for a few callers, but as a general mechanism
> it stinks.  It's a lot of dancing to avoid GFP_ATOMIC allocations; we'd
> be better making idr_get_new() take a gfp_t, and have an idr_pre_alloc()
> for those who care.
> 
> *Sure* there's a chance of racing and we will need to do an atomic
> allocation.  But can anyone justify the current complexity for all
> callers?

Sure, I'm not arguing for the current interface.

> > > + * ida_simple_get - get a new id.
> > > + * @ida: the (initialized) ida.
> > > + * @min_id: the minimum id (inclusive)
> > > + * @max_id: the maximum id (inclusive)
> > > + *
> > > + * Allocates an id in the range min_id <= id <= max_id, or returns -ENOSPC.
> > > + * On allocation failure, returns -ENOMEM.  This function can sleep.
> > > + *
> > > + * Use ida_simple_remove() to get rid of an id.
> > > + */
> > > +int ida_simple_get(struct ida *ida, int min_id, int max_id)
> > 
> > Hmmm... new interface different from existing id[ra] style, but yeah
> > something like the above would have made more sense from the
> > beginning.  The only thing is that isn't (begin <= range < end) more
> > conventional form to express ranges?
> 
> Yes, but how to express an unlimited range then?  I could used unsigned
> and 0x80000000, but that seemed crude.

So, why not do this properly then?  ie. ida_get(ida, begin, end, gfp).

As for the end of range, shouldn't 0 mean the default max range?  Only
some users care about the upper limit anyway.  We can also make it
unsigned just in case and cap the value.

Thanks.

-- 
tejun

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Michael S. Tsirkin]

On Thu, Jun 09, 2011 at 06:41:56AM -0400, Mark Wu wrote:
> On 06/09/2011 05:14 AM, Tejun Heo wrote:
> > Hello,
> > 
> > On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
> >> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> >>> Hi Rusty,
> >>> Yes, I can't figure out an instance of disk probing in parallel either, but as
> >>> per the following commit, I think we still need use lock for safety. What's your opinion?
> >>>
> >>> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> >>> Author: Tejun Heo <tj@kernel.org>
> >>> Date:   Sat Feb 21 11:04:45 2009 +0900
> >>>
> >>>     [SCSI] sd: revive sd_index_lock
> >>>
> >>>     Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> >>>     use ida instead of idr incorrectly removed sd_index_lock around id
> >>>     allocation and free.  idr/ida do have internal locks but they protect
> >>>     their free object lists not the allocation itself.  The caller is
> >>>     responsible for that.  This missing synchronization led to the same id
> >>>     being assigned to multiple devices leading to oops.
> >>
> >> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
> >>
> >> If so, I'll have to review all my drivers.
> > 
> > Unless async is explicitly used, probe happens sequentially.  IOW, if
> > there's no async_schedule() call, things won't happen in parallel.
> > That said, I think it wouldn't be such a bad idea to protect ida with
> > spinlock regardless unless the probe code explicitly requires
> > serialization.
> > 
> > Thanks.
> > 
> Since virtio blk driver doesn't use async probe, it needn't use spinlock to protect ida.
> So remove the lock from patch.
> 
> >From fbb396df9dbf8023f1b268be01b43529a3993d57 Mon Sep 17 00:00:00 2001
> From: Mark Wu <dwu@redhat.com>
> Date: Thu, 9 Jun 2011 06:34:07 -0400
> Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
> 
> Current index allocation in virtio-blk is based on a monotonically
> increasing variable "index". It could cause some confusion about disk
> name in the case of hot-plugging disks. And it's impossible to find the
> lowest available index by just maintaining a simple index. So it's
> changed to use ida to allocate index via referring to the index
> allocation in scsi disk.
> 
> Signed-off-by: Mark Wu <dwu@redhat.com>

Acked-by: Michael S. Tsirkin <mst@redhat.com>

This got lost in the noise and missed 3.1 which is unfortunate.
How about we apply this as is and look at cleanups as a next step?

> ---
>  drivers/block/virtio_blk.c |   28 +++++++++++++++++++++++-----
>  1 files changed, 23 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 079c088..bf81ab6 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -8,10 +8,13 @@
>  #include <linux/scatterlist.h>
>  #include <linux/string_helpers.h>
>  #include <scsi/scsi_cmnd.h>
> +#include <linux/idr.h>
>  
>  #define PART_BITS 4
>  
> -static int major, index;
> +static int major;
> +static DEFINE_IDA(vd_index_ida);
> +
>  struct workqueue_struct *virtblk_wq;
>  
>  struct virtio_blk
> @@ -23,6 +26,7 @@ struct virtio_blk
>  
>  	/* The disk structure for the kernel. */
>  	struct gendisk *disk;
> +	u32 index;
>  
>  	/* Request tracking. */
>  	struct list_head reqs;
> @@ -343,12 +347,23 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	struct request_queue *q;
>  	int err;
>  	u64 cap;
> -	u32 v, blk_size, sg_elems, opt_io_size;
> +	u32 v, blk_size, sg_elems, opt_io_size, index;
>  	u16 min_io_size;
>  	u8 physical_block_exp, alignment_offset;
>  
> -	if (index_to_minor(index) >= 1 << MINORBITS)
> -		return -ENOSPC;
> +	do {
> +		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
> +			return -ENOMEM;
> +		err = ida_get_new(&vd_index_ida, &index);
> +	} while (err == -EAGAIN);
> +
> +	if (err)
> +		return err;
> +
> +	if (index_to_minor(index) >= 1 << MINORBITS) {
> +		err =  -ENOSPC;
> +		goto out_free_index;
> +	}
>  
>  	/* We need to know how many segments before we allocate. */
>  	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
> @@ -421,7 +436,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
>  	vblk->disk->private_data = vblk;
>  	vblk->disk->fops = &virtblk_fops;
>  	vblk->disk->driverfs_dev = &vdev->dev;
> -	index++;
> +	vblk->index = index;
>  
>  	/* configure queue flush support */
>  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
> @@ -516,6 +531,8 @@ out_free_vq:
>  	vdev->config->del_vqs(vdev);
>  out_free_vblk:
>  	kfree(vblk);
> +out_free_index:
> +	ida_remove(&vd_index_ida, index);
>  out:
>  	return err;
>  }
> @@ -538,6 +555,7 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
>  	mempool_destroy(vblk->pool);
>  	vdev->config->del_vqs(vdev);
>  	kfree(vblk);
> +	ida_remove(&vd_index_ida, vblk->index);
>  }
>  
>  static const struct virtio_device_id id_table[] = {
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Jens Axboe]

On 2011-10-24 12:02, Michael S. Tsirkin wrote:
> On Wed, Oct 19, 2011 at 12:12:20PM +0200, Michael S. Tsirkin wrote:
>> On Thu, Jun 09, 2011 at 06:41:56AM -0400, Mark Wu wrote:
>>> On 06/09/2011 05:14 AM, Tejun Heo wrote:
>>>> Hello,
>>>>
>>>> On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
>>>>> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
>>>>>> Hi Rusty,
>>>>>> Yes, I can't figure out an instance of disk probing in parallel either, but as
>>>>>> per the following commit, I think we still need use lock for safety. What's your opinion?
>>>>>>
>>>>>> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
>>>>>> Author: Tejun Heo <tj@kernel.org>
>>>>>> Date:   Sat Feb 21 11:04:45 2009 +0900
>>>>>>
>>>>>>     [SCSI] sd: revive sd_index_lock
>>>>>>
>>>>>>     Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
>>>>>>     use ida instead of idr incorrectly removed sd_index_lock around id
>>>>>>     allocation and free.  idr/ida do have internal locks but they protect
>>>>>>     their free object lists not the allocation itself.  The caller is
>>>>>>     responsible for that.  This missing synchronization led to the same id
>>>>>>     being assigned to multiple devices leading to oops.
>>>>>
>>>>> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
>>>>>
>>>>> If so, I'll have to review all my drivers.
>>>>
>>>> Unless async is explicitly used, probe happens sequentially.  IOW, if
>>>> there's no async_schedule() call, things won't happen in parallel.
>>>> That said, I think it wouldn't be such a bad idea to protect ida with
>>>> spinlock regardless unless the probe code explicitly requires
>>>> serialization.
>>>>
>>>> Thanks.
>>>>
>>> Since virtio blk driver doesn't use async probe, it needn't use spinlock to protect ida.
>>> So remove the lock from patch.
>>>
>>> >From fbb396df9dbf8023f1b268be01b43529a3993d57 Mon Sep 17 00:00:00 2001
>>> From: Mark Wu <dwu@redhat.com>
>>> Date: Thu, 9 Jun 2011 06:34:07 -0400
>>> Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
>>>
>>> Current index allocation in virtio-blk is based on a monotonically
>>> increasing variable "index". It could cause some confusion about disk
>>> name in the case of hot-plugging disks. And it's impossible to find the
>>> lowest available index by just maintaining a simple index. So it's
>>> changed to use ida to allocate index via referring to the index
>>> allocation in scsi disk.
>>>
>>> Signed-off-by: Mark Wu <dwu@redhat.com>
>>
>> Acked-by: Michael S. Tsirkin <mst@redhat.com>
>>
>> This got lost in the noise and missed 3.1 which is unfortunate.
>> How about we apply this as is and look at cleanups as a next step?
> 
> Rusty, any opinion on merging this for 3.2?
> I expect merge window will open right after the summit,

I can toss it into for-3.2/drivers, if there's consensus to do that now.

-- 
Jens Axboe

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Michael S. Tsirkin]

On Wed, Oct 19, 2011 at 12:12:20PM +0200, Michael S. Tsirkin wrote:
> On Thu, Jun 09, 2011 at 06:41:56AM -0400, Mark Wu wrote:
> > On 06/09/2011 05:14 AM, Tejun Heo wrote:
> > > Hello,
> > > 
> > > On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote:
> > >> On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu <dwu@redhat.com> wrote:
> > >>> Hi Rusty,
> > >>> Yes, I can't figure out an instance of disk probing in parallel either, but as
> > >>> per the following commit, I think we still need use lock for safety. What's your opinion?
> > >>>
> > >>> commit 4034cc68157bfa0b6622efe368488d3d3e20f4e6
> > >>> Author: Tejun Heo <tj@kernel.org>
> > >>> Date:   Sat Feb 21 11:04:45 2009 +0900
> > >>>
> > >>>     [SCSI] sd: revive sd_index_lock
> > >>>
> > >>>     Commit f27bac2761cab5a2e212dea602d22457a9aa6943 which converted sd to
> > >>>     use ida instead of idr incorrectly removed sd_index_lock around id
> > >>>     allocation and free.  idr/ida do have internal locks but they protect
> > >>>     their free object lists not the allocation itself.  The caller is
> > >>>     responsible for that.  This missing synchronization led to the same id
> > >>>     being assigned to multiple devices leading to oops.
> > >>
> > >> I'm confused.  Tejun, Greg, anyone can probes happen in parallel?
> > >>
> > >> If so, I'll have to review all my drivers.
> > > 
> > > Unless async is explicitly used, probe happens sequentially.  IOW, if
> > > there's no async_schedule() call, things won't happen in parallel.
> > > That said, I think it wouldn't be such a bad idea to protect ida with
> > > spinlock regardless unless the probe code explicitly requires
> > > serialization.
> > > 
> > > Thanks.
> > > 
> > Since virtio blk driver doesn't use async probe, it needn't use spinlock to protect ida.
> > So remove the lock from patch.
> > 
> > >From fbb396df9dbf8023f1b268be01b43529a3993d57 Mon Sep 17 00:00:00 2001
> > From: Mark Wu <dwu@redhat.com>
> > Date: Thu, 9 Jun 2011 06:34:07 -0400
> > Subject: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
> > 
> > Current index allocation in virtio-blk is based on a monotonically
> > increasing variable "index". It could cause some confusion about disk
> > name in the case of hot-plugging disks. And it's impossible to find the
> > lowest available index by just maintaining a simple index. So it's
> > changed to use ida to allocate index via referring to the index
> > allocation in scsi disk.
> > 
> > Signed-off-by: Mark Wu <dwu@redhat.com>
> 
> Acked-by: Michael S. Tsirkin <mst@redhat.com>
> 
> This got lost in the noise and missed 3.1 which is unfortunate.
> How about we apply this as is and look at cleanups as a next step?

Rusty, any opinion on merging this for 3.2?
I expect merge window will open right after the summit,
so need to decide soon ...

> > ---
> >  drivers/block/virtio_blk.c |   28 +++++++++++++++++++++++-----
> >  1 files changed, 23 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> > index 079c088..bf81ab6 100644
> > --- a/drivers/block/virtio_blk.c
> > +++ b/drivers/block/virtio_blk.c
> > @@ -8,10 +8,13 @@
> >  #include <linux/scatterlist.h>
> >  #include <linux/string_helpers.h>
> >  #include <scsi/scsi_cmnd.h>
> > +#include <linux/idr.h>
> >  
> >  #define PART_BITS 4
> >  
> > -static int major, index;
> > +static int major;
> > +static DEFINE_IDA(vd_index_ida);
> > +
> >  struct workqueue_struct *virtblk_wq;
> >  
> >  struct virtio_blk
> > @@ -23,6 +26,7 @@ struct virtio_blk
> >  
> >  	/* The disk structure for the kernel. */
> >  	struct gendisk *disk;
> > +	u32 index;
> >  
> >  	/* Request tracking. */
> >  	struct list_head reqs;
> > @@ -343,12 +347,23 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> >  	struct request_queue *q;
> >  	int err;
> >  	u64 cap;
> > -	u32 v, blk_size, sg_elems, opt_io_size;
> > +	u32 v, blk_size, sg_elems, opt_io_size, index;
> >  	u16 min_io_size;
> >  	u8 physical_block_exp, alignment_offset;
> >  
> > -	if (index_to_minor(index) >= 1 << MINORBITS)
> > -		return -ENOSPC;
> > +	do {
> > +		if (!ida_pre_get(&vd_index_ida, GFP_KERNEL))
> > +			return -ENOMEM;
> > +		err = ida_get_new(&vd_index_ida, &index);
> > +	} while (err == -EAGAIN);
> > +
> > +	if (err)
> > +		return err;
> > +
> > +	if (index_to_minor(index) >= 1 << MINORBITS) {
> > +		err =  -ENOSPC;
> > +		goto out_free_index;
> > +	}
> >  
> >  	/* We need to know how many segments before we allocate. */
> >  	err = virtio_config_val(vdev, VIRTIO_BLK_F_SEG_MAX,
> > @@ -421,7 +436,7 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
> >  	vblk->disk->private_data = vblk;
> >  	vblk->disk->fops = &virtblk_fops;
> >  	vblk->disk->driverfs_dev = &vdev->dev;
> > -	index++;
> > +	vblk->index = index;
> >  
> >  	/* configure queue flush support */
> >  	if (virtio_has_feature(vdev, VIRTIO_BLK_F_FLUSH))
> > @@ -516,6 +531,8 @@ out_free_vq:
> >  	vdev->config->del_vqs(vdev);
> >  out_free_vblk:
> >  	kfree(vblk);
> > +out_free_index:
> > +	ida_remove(&vd_index_ida, index);
> >  out:
> >  	return err;
> >  }
> > @@ -538,6 +555,7 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
> >  	mempool_destroy(vblk->pool);
> >  	vdev->config->del_vqs(vdev);
> >  	kfree(vblk);
> > +	ida_remove(&vd_index_ida, vblk->index);
> >  }
> >  
> >  static const struct virtio_device_id id_table[] = {
> > -- 
> > 1.7.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe kvm" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index

[Rusty Russell]

On Mon, 24 Oct 2011 12:02:18 +0200, Jens Axboe <axboe@kernel.dk> wrote:
> On 2011-10-24 12:02, Michael S. Tsirkin wrote:
> > On Wed, Oct 19, 2011 at 12:12:20PM +0200, Michael S. Tsirkin wrote:
> > Rusty, any opinion on merging this for 3.2?
> > I expect merge window will open right after the summit,
> 
> I can toss it into for-3.2/drivers, if there's consensus to do that now.

I'd like to see the final patch... we got the new simplified ida stuff
in, so I assume it uses that?

But assume silence from me means consent: it's obviously the Right
Thing.

Thanks,
Rusty.