From mboxrd@z Thu Jan  1 00:00:00 1970
From: David Vrabel <david.vrabel@citrix.com>
Subject: Re: [PATCH] xen-blkback: defer freeing blkif to avoid
 blocking xenwatch
Date: Mon, 12 May 2014 10:39:33 +0100
Message-ID: <537096D5.4090809@citrix.com>
References: <1399678479-14758-1-git-send-email-vali.priescu@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1399678479-14758-1-git-send-email-vali.priescu@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Valentin Priescu <vali.priescu@gmail.com>, xen-devel@lists.xensource.com
Cc: Matt Wilson <msw@amazon.com>, Valentin Priescu <priescuv@amazon.com>
List-Id: xen-devel@lists.xenproject.org

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