Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
On Mon, 24 Oct 2011 12:02:18 +0200, Jens Axboe 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
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 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 >> 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 >>> 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 >> >> Acked-by: Michael S. Tsirkin >> >> 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
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 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
> > >>> 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
> > 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
>
> Acked-by: Michael S. Tsirkin
>
> 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
> > #include
> > #include
> > +#include
> >
> > #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:
> > vde
Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
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 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
> >>> 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
> 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
Acked-by: Michael S. Tsirkin
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
> #include
> #include
> +#include
>
> #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 co
Re: [PATCH 1/1] [virt] virtio-blk: Use ida to allocate disk index
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 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 0x8000, 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
On Wed, 15 Jun 2011 09:06:38 +0200, Tejun Heo 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 0x8000, but that seemed crude. Cheers, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@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
> 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
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
#include
#include
+#include
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
*
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@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
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 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
>>> 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
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
---
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
#include
#include
+#include
#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 majord...@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
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 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 > > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
On Thu, Jun 09, 2011 at 08:51:05AM +0930, Rusty Russell wrote: > On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu 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 > > 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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
On Wed, 08 Jun 2011 09:08:29 -0400, Mark Wu 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 > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
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 majord...@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
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
---
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
#include
#include
+#include
#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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@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
On 06/01/2011 07:57 PM, Rusty Russell wrote: > On Wed, 1 Jun 2011 03:24:29 -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 > > 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 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
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
> ---
> 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
> #include
> #include
> +#include
>
> #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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@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
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 majord...@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
On Wed, 1 Jun 2011 03:24:29 -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 Hi Mark, I don't believe that we do disk probes in parallel, so the spinlock is unnecessary. Otherwise, this looks good. Thanks, Rusty. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@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
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;
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
