* Re: [PATCH] xen-blkfront: make blkif_io_lock spinlock per-device
[not found] <1329509084-26013-1-git-send-email-snoonan@amazon.com>
@ 2012-02-21 14:58 ` Konrad Rzeszutek Wilk
0 siblings, 0 replies; only message in thread
From: Konrad Rzeszutek Wilk @ 2012-02-21 14:58 UTC (permalink / raw)
To: Steven Noonan; +Cc: xen-devel
On Fri, Feb 17, 2012 at 12:04:44PM -0800, Steven Noonan wrote:
> This patch moves the global blkif_io_lock to the per-device structure. The
> spinlock seems to exists for two reasons: to disable IRQs when in the interrupt
> handlers for blkfront, and to protect the blkfront VBDs when a detachment is
> requested.
>
> Having a global blkif_io_lock doesn't make sense given the use case, and it
> drastically hinders performance due to contention. All VBDs with pending IOs
> have to take the lock in order to get work done, which serializes everything
> pretty badly.
applied in #testing.
>
> Signed-off-by: Steven Noonan <snoonan@amazon.com>
> ---
> drivers/block/xen-blkfront.c | 32 ++++++++++++++++----------------
> 1 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index 7b2ec59..df9b8da 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -81,6 +81,7 @@ static const struct block_device_operations xlvbd_block_fops;
> */
> struct blkfront_info
> {
> + spinlock_t io_lock;
> struct mutex mutex;
> struct xenbus_device *xbdev;
> struct gendisk *gd;
> @@ -104,8 +105,6 @@ struct blkfront_info
> int is_ready;
> };
>
> -static DEFINE_SPINLOCK(blkif_io_lock);
> -
> static unsigned int nr_minors;
> static unsigned long *minors;
> static DEFINE_SPINLOCK(minor_lock);
> @@ -413,7 +412,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
> struct request_queue *rq;
> struct blkfront_info *info = gd->private_data;
>
> - rq = blk_init_queue(do_blkif_request, &blkif_io_lock);
> + rq = blk_init_queue(do_blkif_request, &info->io_lock);
> if (rq == NULL)
> return -1;
>
> @@ -628,14 +627,14 @@ static void xlvbd_release_gendisk(struct blkfront_info *info)
> if (info->rq == NULL)
> return;
>
> - spin_lock_irqsave(&blkif_io_lock, flags);
> + spin_lock_irqsave(&info->io_lock, flags);
>
> /* No more blkif_request(). */
> blk_stop_queue(info->rq);
>
> /* No more gnttab callback work. */
> gnttab_cancel_free_callback(&info->callback);
> - spin_unlock_irqrestore(&blkif_io_lock, flags);
> + spin_unlock_irqrestore(&info->io_lock, flags);
>
> /* Flush gnttab callback work. Must be done with no locks held. */
> flush_work_sync(&info->work);
> @@ -667,16 +666,16 @@ static void blkif_restart_queue(struct work_struct *work)
> {
> struct blkfront_info *info = container_of(work, struct blkfront_info, work);
>
> - spin_lock_irq(&blkif_io_lock);
> + spin_lock_irq(&info->io_lock);
> if (info->connected == BLKIF_STATE_CONNECTED)
> kick_pending_request_queues(info);
> - spin_unlock_irq(&blkif_io_lock);
> + spin_unlock_irq(&info->io_lock);
> }
>
> static void blkif_free(struct blkfront_info *info, int suspend)
> {
> /* Prevent new requests being issued until we fix things up. */
> - spin_lock_irq(&blkif_io_lock);
> + spin_lock_irq(&info->io_lock);
> info->connected = suspend ?
> BLKIF_STATE_SUSPENDED : BLKIF_STATE_DISCONNECTED;
> /* No more blkif_request(). */
> @@ -684,7 +683,7 @@ static void blkif_free(struct blkfront_info *info, int suspend)
> blk_stop_queue(info->rq);
> /* No more gnttab callback work. */
> gnttab_cancel_free_callback(&info->callback);
> - spin_unlock_irq(&blkif_io_lock);
> + spin_unlock_irq(&info->io_lock);
>
> /* Flush gnttab callback work. Must be done with no locks held. */
> flush_work_sync(&info->work);
> @@ -718,10 +717,10 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
> struct blkfront_info *info = (struct blkfront_info *)dev_id;
> int error;
>
> - spin_lock_irqsave(&blkif_io_lock, flags);
> + spin_lock_irqsave(&info->io_lock, flags);
>
> if (unlikely(info->connected != BLKIF_STATE_CONNECTED)) {
> - spin_unlock_irqrestore(&blkif_io_lock, flags);
> + spin_unlock_irqrestore(&info->io_lock, flags);
> return IRQ_HANDLED;
> }
>
> @@ -803,7 +802,7 @@ static irqreturn_t blkif_interrupt(int irq, void *dev_id)
>
> kick_pending_request_queues(info);
>
> - spin_unlock_irqrestore(&blkif_io_lock, flags);
> + spin_unlock_irqrestore(&info->io_lock, flags);
>
> return IRQ_HANDLED;
> }
> @@ -978,6 +977,7 @@ static int blkfront_probe(struct xenbus_device *dev,
> }
>
> mutex_init(&info->mutex);
> + spin_lock_init(&info->io_lock);
> info->xbdev = dev;
> info->vdevice = vdevice;
> info->connected = BLKIF_STATE_DISCONNECTED;
> @@ -1053,7 +1053,7 @@ static int blkif_recover(struct blkfront_info *info)
>
> xenbus_switch_state(info->xbdev, XenbusStateConnected);
>
> - spin_lock_irq(&blkif_io_lock);
> + spin_lock_irq(&info->io_lock);
>
> /* Now safe for us to use the shared ring */
> info->connected = BLKIF_STATE_CONNECTED;
> @@ -1064,7 +1064,7 @@ static int blkif_recover(struct blkfront_info *info)
> /* Kick any other new requests queued since we resumed */
> kick_pending_request_queues(info);
>
> - spin_unlock_irq(&blkif_io_lock);
> + spin_unlock_irq(&info->io_lock);
>
> return 0;
> }
> @@ -1254,10 +1254,10 @@ static void blkfront_connect(struct blkfront_info *info)
> xenbus_switch_state(info->xbdev, XenbusStateConnected);
>
> /* Kick pending requests. */
> - spin_lock_irq(&blkif_io_lock);
> + spin_lock_irq(&info->io_lock);
> info->connected = BLKIF_STATE_CONNECTED;
> kick_pending_request_queues(info);
> - spin_unlock_irq(&blkif_io_lock);
> + spin_unlock_irq(&info->io_lock);
>
> add_disk(info->gd);
>
> --
> 1.7.9
^ permalink raw reply [flat|nested] only message in thread