[PATCH] xen/blkback: Squash the discard support for 'file' and 'phy' type.

[PATCH] xen/blkback: Squash the discard support for 'file' and 'phy' type.

[Konrad Rzeszutek Wilk]

The only reason for the distinction was for the special case of
'file' (which is assumed to be loopback device), was to reach inside
the loopback device, find the underlaying file, and call fallocate on it.
Fortunately "xen-blkback: convert hole punching to discard request on
loop devices" removes that use-case and we now based the discard
support based on blk_queue_discard(q) and extract all appropriate
parameters from the 'struct request_queue'.

CC: Li Dongyang <lidongyang@novell.com>
CC: Jan Beulich <JBeulich@suse.com>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 drivers/block/xen-blkback/blkback.c |   20 ++++------
 drivers/block/xen-blkback/common.h  |    6 ---
 drivers/block/xen-blkback/xenbus.c  |   67 +++++++++++++---------------------
 3 files changed, 34 insertions(+), 59 deletions(-)

diff --git a/drivers/block/xen-blkback/blkback.c b/drivers/block/xen-blkback/blkback.c
index 0088bf6..1a83f39 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -419,21 +419,17 @@ static int dispatch_discard_io(struct xen_blkif *blkif,
 	int err = 0;
 	int status = BLKIF_RSP_OKAY;
 	struct block_device *bdev = blkif->vbd.bdev;
-
+	unsigned long secure = 0;
 	blkif->st_ds_req++;
 
 	xen_blkif_get(blkif);
-	if (blkif->blk_backend_type == BLKIF_BACKEND_PHY ||
-	    blkif->blk_backend_type == BLKIF_BACKEND_FILE) {
-		unsigned long secure = (blkif->vbd.discard_secure &&
-			(req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
-			BLKDEV_DISCARD_SECURE : 0;
-		err = blkdev_issue_discard(bdev,
-				req->u.discard.sector_number,
-				req->u.discard.nr_sectors,
-				GFP_KERNEL, secure);
-	} else
-		err = -EOPNOTSUPP;
+	secure = (blkif->vbd.discard_secure &&
+		 (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
+		 BLKDEV_DISCARD_SECURE : 0;
+
+	err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
+				   req->u.discard.nr_sectors,
+				   GFP_KERNEL, secure);
 
 	if (err == -EOPNOTSUPP) {
 		pr_debug(DRV_PFX "discard op failed, not supported\n");
diff --git a/drivers/block/xen-blkback/common.h b/drivers/block/xen-blkback/common.h
index d0ee7ed..773cf27 100644
--- a/drivers/block/xen-blkback/common.h
+++ b/drivers/block/xen-blkback/common.h
@@ -146,11 +146,6 @@ enum blkif_protocol {
 	BLKIF_PROTOCOL_X86_64 = 3,
 };
 
-enum blkif_backend_type {
-	BLKIF_BACKEND_PHY  = 1,
-	BLKIF_BACKEND_FILE = 2,
-};
-
 struct xen_vbd {
 	/* What the domain refers to this vbd as. */
 	blkif_vdev_t		handle;
@@ -177,7 +172,6 @@ struct xen_blkif {
 	unsigned int		irq;
 	/* Comms information. */
 	enum blkif_protocol	blk_protocol;
-	enum blkif_backend_type blk_backend_type;
 	union blkif_back_rings	blk_rings;
 	void			*blk_ring;
 	/* The VBD attached to this interface. */
diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 24a2fb5..9aad6de 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -393,52 +393,37 @@ int xen_blkbk_discard(struct xenbus_transaction xbt, struct backend_info *be)
 	char *type;
 	int err;
 	int state = 0;
+	struct block_device *bdev = be->blkif->vbd.bdev;
+	struct request_queue *q = bdev_get_queue(bdev);
 
-	type = xenbus_read(XBT_NIL, dev->nodename, "type", NULL);
-	if (!IS_ERR(type)) {
-		if (strncmp(type, "file", 4) == 0) {
-			state = 1;
-			blkif->blk_backend_type = BLKIF_BACKEND_FILE;
+	if (blk_queue_discard(q)) {
+		err = xenbus_printf(xbt, dev->nodename,
+			"discard-granularity", "%u",
+			q->limits.discard_granularity);
+		if (err) {
+			xenbus_dev_fatal(dev, err,
+				"writing discard-granularity");
+			goto kfree;
+		}
+		err = xenbus_printf(xbt, dev->nodename,
+			"discard-alignment", "%u",
+			q->limits.discard_alignment);
+		if (err) {
+			xenbus_dev_fatal(dev, err,
+				"writing discard-alignment");
+			goto kfree;
 		}
-		if (strncmp(type, "phy", 3) == 0) {
-			struct block_device *bdev = be->blkif->vbd.bdev;
-			struct request_queue *q = bdev_get_queue(bdev);
-			if (blk_queue_discard(q)) {
-				err = xenbus_printf(xbt, dev->nodename,
-					"discard-granularity", "%u",
-					q->limits.discard_granularity);
-				if (err) {
-					xenbus_dev_fatal(dev, err,
-						"writing discard-granularity");
-					goto kfree;
-				}
-				err = xenbus_printf(xbt, dev->nodename,
-					"discard-alignment", "%u",
-					q->limits.discard_alignment);
-				if (err) {
-					xenbus_dev_fatal(dev, err,
-						"writing discard-alignment");
-					goto kfree;
-				}
-				state = 1;
-				blkif->blk_backend_type = BLKIF_BACKEND_PHY;
-			}
-			/* Optional. */
-			err = xenbus_printf(xbt, dev->nodename,
-				"discard-secure", "%d",
-				blkif->vbd.discard_secure);
-			if (err) {
-				xenbus_dev_fatal(dev, err,
+		state = 1;
+		/* Optional. */
+		err = xenbus_printf(xbt, dev->nodename,
+				    "discard-secure", "%d",
+				    blkif->vbd.discard_secure);
+		if (err) {
+			xenbus_dev_fatal(dev, err,
 					"writting discard-secure");
-				goto kfree;
-			}
+			goto kfree;
 		}
-	} else {
-		err = PTR_ERR(type);
-		xenbus_dev_fatal(dev, err, "reading type");
-		goto out;
 	}
-
 	err = xenbus_printf(xbt, dev->nodename, "feature-discard",
 			    "%d", state);
 	if (err)
-- 
1.7.9.48.g85da4d

Re: [Xen-devel] [PATCH] xen/blkback: Squash the discard support for 'file' and 'phy' type.

[Ian Campbell]

On Tue, 2012-03-13 at 22:52 +0000, Konrad Rzeszutek Wilk wrote:
> The only reason for the distinction was for the special case of
> 'file' (which is assumed to be loopback device)

Not related to this patch but how feasible would it be to make blkback
able to cope with files directly?

Ian.

Re: [Xen-devel] [PATCH] xen/blkback: Squash the discard support for 'file' and 'phy' type.

[Konrad Rzeszutek Wilk]

On Wed, Mar 14, 2012 at 08:33:55AM +0000, Ian Campbell wrote:
> On Tue, 2012-03-13 at 22:52 +0000, Konrad Rzeszutek Wilk wrote:
> > The only reason for the distinction was for the special case of
> > 'file' (which is assumed to be loopback device)
> 
> Not related to this patch but how feasible would it be to make blkback
> able to cope with files directly?

Intersting thought. I am not sure... but an interesting thought. Would you
have Daniel's private email? We could email him for thoughts.
> 
> Ian.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH] xen/blkback: Squash the discard support for 'file' and 'phy' type.

[Ian Campbell]

On Wed, 2012-03-14 at 20:23 +0000, Konrad Rzeszutek Wilk wrote:
> On Wed, Mar 14, 2012 at 08:33:55AM +0000, Ian Campbell wrote:
> > On Tue, 2012-03-13 at 22:52 +0000, Konrad Rzeszutek Wilk wrote:
> > > The only reason for the distinction was for the special case of
> > > 'file' (which is assumed to be loopback device)
> > 
> > Not related to this patch but how feasible would it be to make blkback
> > able to cope with files directly?
> 
> Intersting thought. I am not sure... but an interesting thought. Would you
> have Daniel's private email? We could email him for thoughts.

I'm afraid I don't have any contact info for Daniel these days.

Ian.

Re: [PATCH] xen/blkback: Squash the discard support for 'file' and 'phy' type.

[Jan Beulich]

>>> On 13.03.12 at 23:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> The only reason for the distinction was for the special case of
> 'file' (which is assumed to be loopback device), was to reach inside
> the loopback device, find the underlaying file, and call fallocate on it.
> Fortunately "xen-blkback: convert hole punching to discard request on
> loop devices" removes that use-case and we now based the discard
> support based on blk_queue_discard(q) and extract all appropriate
> parameters from the 'struct request_queue'.
> 
> CC: Li Dongyang <lidongyang@novell.com>
> CC: Jan Beulich <JBeulich@suse.com>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

(with a few minor remarks below)

> --- a/drivers/block/xen-blkback/blkback.c
> +++ b/drivers/block/xen-blkback/blkback.c
> @@ -419,21 +419,17 @@ static int dispatch_discard_io(struct xen_blkif *blkif,
>  	int err = 0;
>  	int status = BLKIF_RSP_OKAY;
>  	struct block_device *bdev = blkif->vbd.bdev;
> -
> +	unsigned long secure = 0;

Mind keeping the blank line and dropping the pointless initializer (which
future gcc is likely going to be warning about)?

>  	blkif->st_ds_req++;
>  
>  	xen_blkif_get(blkif);
> -	if (blkif->blk_backend_type == BLKIF_BACKEND_PHY ||
> -	    blkif->blk_backend_type == BLKIF_BACKEND_FILE) {
> -		unsigned long secure = (blkif->vbd.discard_secure &&
> -			(req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
> -			BLKDEV_DISCARD_SECURE : 0;
> -		err = blkdev_issue_discard(bdev,
> -				req->u.discard.sector_number,
> -				req->u.discard.nr_sectors,
> -				GFP_KERNEL, secure);
> -	} else
> -		err = -EOPNOTSUPP;
> +	secure = (blkif->vbd.discard_secure &&
> +		 (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
> +		 BLKDEV_DISCARD_SECURE : 0;
> +
> +	err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
> +				   req->u.discard.nr_sectors,
> +				   GFP_KERNEL, secure);
>  
>  	if (err == -EOPNOTSUPP) {
>  		pr_debug(DRV_PFX "discard op failed, not supported\n");
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -393,52 +393,37 @@ int xen_blkbk_discard(struct xenbus_transaction xbt, 
> struct backend_info *be)
>  	char *type;
>  	int err;
>  	int state = 0;
> +	struct block_device *bdev = be->blkif->vbd.bdev;
> +	struct request_queue *q = bdev_get_queue(bdev);
>  
> -	type = xenbus_read(XBT_NIL, dev->nodename, "type", NULL);
> -	if (!IS_ERR(type)) {
> -		if (strncmp(type, "file", 4) == 0) {
> -			state = 1;
> -			blkif->blk_backend_type = BLKIF_BACKEND_FILE;
> +	if (blk_queue_discard(q)) {
> +		err = xenbus_printf(xbt, dev->nodename,
> +			"discard-granularity", "%u",
> +			q->limits.discard_granularity);
> +		if (err) {
> +			xenbus_dev_fatal(dev, err,
> +				"writing discard-granularity");
> +			goto kfree;

Unrelated to the patch here, but failure to write any sort of extension
data shouldn't be considered fatal - the backend can well work without
these, and it should be left to the frontend to decide whether it wants
to live without the unavailable extensions.

Jan

> +		}
> +		err = xenbus_printf(xbt, dev->nodename,
> +			"discard-alignment", "%u",
> +			q->limits.discard_alignment);
> +		if (err) {
> +			xenbus_dev_fatal(dev, err,
> +				"writing discard-alignment");
> +			goto kfree;
>  		}
> -		if (strncmp(type, "phy", 3) == 0) {
> -			struct block_device *bdev = be->blkif->vbd.bdev;
> -			struct request_queue *q = bdev_get_queue(bdev);
> -			if (blk_queue_discard(q)) {
> -				err = xenbus_printf(xbt, dev->nodename,
> -					"discard-granularity", "%u",
> -					q->limits.discard_granularity);
> -				if (err) {
> -					xenbus_dev_fatal(dev, err,
> -						"writing discard-granularity");
> -					goto kfree;
> -				}
> -				err = xenbus_printf(xbt, dev->nodename,
> -					"discard-alignment", "%u",
> -					q->limits.discard_alignment);
> -				if (err) {
> -					xenbus_dev_fatal(dev, err,
> -						"writing discard-alignment");
> -					goto kfree;
> -				}
> -				state = 1;
> -				blkif->blk_backend_type = BLKIF_BACKEND_PHY;
> -			}
> -			/* Optional. */
> -			err = xenbus_printf(xbt, dev->nodename,
> -				"discard-secure", "%d",
> -				blkif->vbd.discard_secure);
> -			if (err) {
> -				xenbus_dev_fatal(dev, err,
> +		state = 1;
> +		/* Optional. */
> +		err = xenbus_printf(xbt, dev->nodename,
> +				    "discard-secure", "%d",
> +				    blkif->vbd.discard_secure);
> +		if (err) {
> +			xenbus_dev_fatal(dev, err,
>  					"writting discard-secure");
> -				goto kfree;
> -			}
> +			goto kfree;
>  		}
> -	} else {
> -		err = PTR_ERR(type);
> -		xenbus_dev_fatal(dev, err, "reading type");
> -		goto out;
>  	}
> -
>  	err = xenbus_printf(xbt, dev->nodename, "feature-discard",
>  			    "%d", state);
>  	if (err)

Re: [PATCH] xen/blkback: Squash the discard support for 'file' and 'phy' type.

[Konrad Rzeszutek Wilk]

On Wed, Mar 14, 2012 at 09:48:21AM +0000, Jan Beulich wrote:
> >>> On 13.03.12 at 23:52, Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> > The only reason for the distinction was for the special case of
> > 'file' (which is assumed to be loopback device), was to reach inside
> > the loopback device, find the underlaying file, and call fallocate on it.
> > Fortunately "xen-blkback: convert hole punching to discard request on
> > loop devices" removes that use-case and we now based the discard
> > support based on blk_queue_discard(q) and extract all appropriate
> > parameters from the 'struct request_queue'.
> > 
> > CC: Li Dongyang <lidongyang@novell.com>
> > CC: Jan Beulich <JBeulich@suse.com>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> Acked-by: Jan Beulich <jbeulich@suse.com>
> 
> (with a few minor remarks below)
> 
> > --- a/drivers/block/xen-blkback/blkback.c
> > +++ b/drivers/block/xen-blkback/blkback.c
> > @@ -419,21 +419,17 @@ static int dispatch_discard_io(struct xen_blkif *blkif,
> >  	int err = 0;
> >  	int status = BLKIF_RSP_OKAY;
> >  	struct block_device *bdev = blkif->vbd.bdev;
> > -
> > +	unsigned long secure = 0;
> 
> Mind keeping the blank line and dropping the pointless initializer (which
> future gcc is likely going to be warning about)?

<nods>
> 
> >  	blkif->st_ds_req++;
> >  
> >  	xen_blkif_get(blkif);
> > -	if (blkif->blk_backend_type == BLKIF_BACKEND_PHY ||
> > -	    blkif->blk_backend_type == BLKIF_BACKEND_FILE) {
> > -		unsigned long secure = (blkif->vbd.discard_secure &&
> > -			(req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
> > -			BLKDEV_DISCARD_SECURE : 0;
> > -		err = blkdev_issue_discard(bdev,
> > -				req->u.discard.sector_number,
> > -				req->u.discard.nr_sectors,
> > -				GFP_KERNEL, secure);
> > -	} else
> > -		err = -EOPNOTSUPP;
> > +	secure = (blkif->vbd.discard_secure &&
> > +		 (req->u.discard.flag & BLKIF_DISCARD_SECURE)) ?
> > +		 BLKDEV_DISCARD_SECURE : 0;
> > +
> > +	err = blkdev_issue_discard(bdev, req->u.discard.sector_number,
> > +				   req->u.discard.nr_sectors,
> > +				   GFP_KERNEL, secure);
> >  
> >  	if (err == -EOPNOTSUPP) {
> >  		pr_debug(DRV_PFX "discard op failed, not supported\n");
> > --- a/drivers/block/xen-blkback/xenbus.c
> > +++ b/drivers/block/xen-blkback/xenbus.c
> > @@ -393,52 +393,37 @@ int xen_blkbk_discard(struct xenbus_transaction xbt, 
> > struct backend_info *be)
> >  	char *type;
> >  	int err;
> >  	int state = 0;
> > +	struct block_device *bdev = be->blkif->vbd.bdev;
> > +	struct request_queue *q = bdev_get_queue(bdev);
> >  
> > -	type = xenbus_read(XBT_NIL, dev->nodename, "type", NULL);
> > -	if (!IS_ERR(type)) {
> > -		if (strncmp(type, "file", 4) == 0) {
> > -			state = 1;
> > -			blkif->blk_backend_type = BLKIF_BACKEND_FILE;
> > +	if (blk_queue_discard(q)) {
> > +		err = xenbus_printf(xbt, dev->nodename,
> > +			"discard-granularity", "%u",
> > +			q->limits.discard_granularity);
> > +		if (err) {
> > +			xenbus_dev_fatal(dev, err,
> > +				"writing discard-granularity");
> > +			goto kfree;
> 
> Unrelated to the patch here, but failure to write any sort of extension
> data shouldn't be considered fatal - the backend can well work without
> these, and it should be left to the frontend to decide whether it wants
> to live without the unavailable extensions.

<nods>. Let me whip up another patch that removes these 'fatal' cases
and is based on this one.

> 
> Jan
> 
> > +		}
> > +		err = xenbus_printf(xbt, dev->nodename,
> > +			"discard-alignment", "%u",
> > +			q->limits.discard_alignment);
> > +		if (err) {
> > +			xenbus_dev_fatal(dev, err,
> > +				"writing discard-alignment");
> > +			goto kfree;
> >  		}
> > -		if (strncmp(type, "phy", 3) == 0) {
> > -			struct block_device *bdev = be->blkif->vbd.bdev;
> > -			struct request_queue *q = bdev_get_queue(bdev);
> > -			if (blk_queue_discard(q)) {
> > -				err = xenbus_printf(xbt, dev->nodename,
> > -					"discard-granularity", "%u",
> > -					q->limits.discard_granularity);
> > -				if (err) {
> > -					xenbus_dev_fatal(dev, err,
> > -						"writing discard-granularity");
> > -					goto kfree;
> > -				}
> > -				err = xenbus_printf(xbt, dev->nodename,
> > -					"discard-alignment", "%u",
> > -					q->limits.discard_alignment);
> > -				if (err) {
> > -					xenbus_dev_fatal(dev, err,
> > -						"writing discard-alignment");
> > -					goto kfree;
> > -				}
> > -				state = 1;
> > -				blkif->blk_backend_type = BLKIF_BACKEND_PHY;
> > -			}
> > -			/* Optional. */
> > -			err = xenbus_printf(xbt, dev->nodename,
> > -				"discard-secure", "%d",
> > -				blkif->vbd.discard_secure);
> > -			if (err) {
> > -				xenbus_dev_fatal(dev, err,
> > +		state = 1;
> > +		/* Optional. */
> > +		err = xenbus_printf(xbt, dev->nodename,
> > +				    "discard-secure", "%d",
> > +				    blkif->vbd.discard_secure);
> > +		if (err) {
> > +			xenbus_dev_fatal(dev, err,
> >  					"writting discard-secure");
> > -				goto kfree;
> > -			}
> > +			goto kfree;
> >  		}
> > -	} else {
> > -		err = PTR_ERR(type);
> > -		xenbus_dev_fatal(dev, err, "reading type");
> > -		goto out;
> >  	}
> > -
> >  	err = xenbus_printf(xbt, dev->nodename, "feature-discard",
> >  			    "%d", state);
> >  	if (err)
>