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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.03.12 at 23:52, Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 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@xxxxxxxxxx>
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

(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)



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel