Re: [PATCH] xen-blkback: defer freeing blkif to avoid blocking xenwatch

[David Vrabel <david.vrabel@citrix.com>]

On 10/05/14 00:34, Valentin Priescu wrote:
> From: Valentin Priescu <priescuv@amazon.com>
> 
> Currently xenwatch blocks in VBD disconnect, waiting for all pending I/O
> requests to finish. If the VBD is attached to a hot-swappable disk, then
> xenwatch can hang for a long period of time, stalling other watches.
>
[...]
> 
> With this patch, when there is still pending I/O, the actual disconnect
> is done by the last reference holder (last pending I/O request). In this
> case, xenwatch doesn't block indefinitely.

[...]

> @@ -239,19 +253,26 @@ static void xen_blkif_disconnect(struct xen_blkif *blkif)
>  		blkif->xenblkd = NULL;
>  	}
>  
> -	atomic_dec(&blkif->refcnt);
> -	wait_event(blkif->waiting_to_free, atomic_read(&blkif->refcnt) == 0);
> -	atomic_inc(&blkif->refcnt);
> +	/* The above kthread_stop() guarantees that at this point we
> +	 * don't have any discard_io or other_io requests. So, checking
> +	 * for inflight IO is enough.
> +	 */
> +	if (!atomic_read(&blkif->inflight)) {

if (atomic_read(&blkif->inflight > 0)
    return -EBUSY;

> +		if (blkif->irq) {
> +			unbind_from_irqhandler(blkif->irq, blkif);
> +			blkif->irq = 0;
> +		}
>  
> -	if (blkif->irq) {
> -		unbind_from_irqhandler(blkif->irq, blkif);
> -		blkif->irq = 0;
> -	}
> +		if (blkif->blk_rings.common.sring) {
> +			xenbus_unmap_ring_vfree(blkif->be->dev,
> +				blkif->blk_ring);
> +			blkif->blk_rings.common.sring = NULL;
> +		}
>  
> -	if (blkif->blk_rings.common.sring) {
> -		xenbus_unmap_ring_vfree(blkif->be->dev, blkif->blk_ring);
> -		blkif->blk_rings.common.sring = NULL;
> +		return 0;
>  	}
> +
> +	return -EBUSY;
>  }
[...]
> @@ -700,7 +720,9 @@ static void frontend_changed(struct xenbus_device *dev,
>  		 * Enforce precondition before potential leak point.
>  		 * xen_blkif_disconnect() is idempotent.
>  		 */
> -		xen_blkif_disconnect(be->blkif);
> +		err = xen_blkif_disconnect(be->blkif);
> +		if (err)
> +			break;
>  
>  		err = connect_ring(be);
>  		if (err)

If xen_blkif_disconnect() returns -EBUSY, nothing will trigger the
reconnect.

David