From: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
To: Stefan Bader <stefan.bader@canonical.com>
Cc: "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
"Jan Beulich" <jbeulich@suse.com>,
"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [PATCH v3] xen-blk(front|back): Handle large physical sector disks
Date: Tue, 28 May 2013 08:47:56 -0400 [thread overview]
Message-ID: <20130528124756.GA3754@phenom.dumpdata.com> (raw)
In-Reply-To: <5193A38B.4080508@canonical.com>
On Wed, May 15, 2013 at 05:02:35PM +0200, Stefan Bader wrote:
> Changed the xenbus_gather but kept the xenbus_dev_error (without being fatal) as
> that seems closer to the style of the other calls.
Hm, could you rebase this on top of stable/for-jens-3.10 please?
I get this:
patching file drivers/block/xen-blkback/xenbus.c
Hunk #1 FAILED at 704.
1 out of 1 hunk FAILED -- saving rejects to file drivers/block/xen-blkback/xenbus.c.rej
patching file drivers/block/xen-blkfront.c
Hunk #1 FAILED at 542.
patch: **** malformed patch at line 133: 16
>
> -Stefan
>
> From cddea87d842cc5feb427f3eedefc0566d69fb9b6 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Mon, 13 May 2013 16:28:15 +0200
> Subject: [PATCH] xen/blk: Use physical sector size for setup
>
> Currently xen-blkback passes the logical sector size over xenbus and
> xen-blkfront sets up the paravirt disk with that logical block size.
> But newer drives usually have the logical sector size set to 512 for
> compatibility reasons and would show the actual sector size only in
> physical sector size.
> This results in the device being partitioned and accessed in dom0 with
> the correct sector size, but the guest thinks 512 bytes is the correct
> block size. And that results in poor performance.
>
> To fix this, blkback gets modified to pass also physical-sector-size
> over xenbus and blkfront to use both values to set up the paravirt
> disk. I did not just change the passed in sector-size because I am
> not sure having a bigger logical sector size than the physical one
> is valid (and that would happen if a newer dom0 kernel hits an older
> domU kernel). Also this way a domU set up before should still be
> accessible (just some tools might detect the unaligned setup).
>
> [v2: Make xenbus write failure non-fatal]
> [v3: Use xenbus_scanf instead of xenbus_gather]
>
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 5 +++++
> drivers/block/xen-blkfront.c | 23 +++++++++++++++++++----
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 8bfd1bc..25463e3 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -704,6 +704,11 @@ again:
> dev->nodename);
> goto abort;
> }
> + err = xenbus_printf(xbt, dev->nodename, "physical-sector-size", "%u",
> + bdev_physical_block_size(be->blkif->vbd.bdev));
> + if (err)
> + xenbus_dev_error(dev, err, "writing %s/physical-sector-size",
> + dev->nodename);
>
> err = xenbus_transaction_end(xbt, 0);
> if (err == -EAGAIN)
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index d89ef86..33b5618 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -542,7 +542,8 @@ wait:
> flush_requests(info);
> }
>
> -static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
> +static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
> + unsigned int physical_sector_size)
> {
> struct request_queue *rq;
> struct blkfront_info *info = gd->private_data;
> @@ -564,6 +565,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16
> sector_size)
>
> /* Hard sector size and max sectors impersonate the equiv. hardware. */
> blk_queue_logical_block_size(rq, sector_size);
> + blk_queue_physical_block_size(rq, physical_sector_size);
> blk_queue_max_hw_sectors(rq, 512);
>
> /* Each segment in a request is up to an aligned page in size. */
> @@ -667,7 +669,8 @@ static char *encode_disk_name(char *ptr, unsigned int n)
>
> static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
> struct blkfront_info *info,
> - u16 vdisk_info, u16 sector_size)
> + u16 vdisk_info, u16 sector_size,
> + unsigned int physical_sector_size)
> {
> struct gendisk *gd;
> int nr_minors = 1;
> @@ -734,7 +737,7 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
> gd->driverfs_dev = &(info->xbdev->dev);
> set_capacity(gd, capacity);
>
> - if (xlvbd_init_blk_queue(gd, sector_size)) {
> + if (xlvbd_init_blk_queue(gd, sector_size, physical_sector_size)) {
> del_gendisk(gd);
> goto release;
> }
> @@ -1395,6 +1398,7 @@ static void blkfront_connect(struct blkfront_info *info)
> {
> unsigned long long sectors;
> unsigned long sector_size;
> + unsigned int physical_sector_size;
> unsigned int binfo;
> int err;
> int barrier, flush, discard, persistent;
> @@ -1437,6 +1441,16 @@ static void blkfront_connect(struct blkfront_info *info)
> return;
> }
>
> + /*
> + * physcial-sector-size is a newer field, so old backends may not
> + * provide this. Assume physical sector size to be the same as
> + * sector_size in that case.
> + */
> + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
> + "physical-sector-size", "%u", &physical_sector_size);
> + if (err != 1)
> + physical_sector_size = sector_size;
> +
> info->feature_flush = 0;
> info->flush_op = 0;
>
> @@ -1483,7 +1497,8 @@ static void blkfront_connect(struct blkfront_info *info)
> else
> info->feature_persistent = persistent;
>
> - err = xlvbd_alloc_gendisk(sectors, info, binfo, sector_size);
> + err = xlvbd_alloc_gendisk(sectors, info, binfo, sector_size,
> + physical_sector_size);
> if (err) {
> xenbus_dev_fatal(info->xbdev, err, "xlvbd_add at %s",
> info->xbdev->otherend);
> --
> 1.7.9.5
>
> From cddea87d842cc5feb427f3eedefc0566d69fb9b6 Mon Sep 17 00:00:00 2001
> From: Stefan Bader <stefan.bader@canonical.com>
> Date: Mon, 13 May 2013 16:28:15 +0200
> Subject: [PATCH] xen/blk: Use physical sector size for setup
>
> Currently xen-blkback passes the logical sector size over xenbus and
> xen-blkfront sets up the paravirt disk with that logical block size.
> But newer drives usually have the logical sector size set to 512 for
> compatibility reasons and would show the actual sector size only in
> physical sector size.
> This results in the device being partitioned and accessed in dom0 with
> the correct sector size, but the guest thinks 512 bytes is the correct
> block size. And that results in poor performance.
>
> To fix this, blkback gets modified to pass also physical-sector-size
> over xenbus and blkfront to use both values to set up the paravirt
> disk. I did not just change the passed in sector-size because I am
> not sure having a bigger logical sector size than the physical one
> is valid (and that would happen if a newer dom0 kernel hits an older
> domU kernel). Also this way a domU set up before should still be
> accessible (just some tools might detect the unaligned setup).
>
> [v2: Make xenbus write failure non-fatal]
> [v3: Use xenbus_scanf instead of xenbus_gather]
>
> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
> ---
> drivers/block/xen-blkback/xenbus.c | 5 +++++
> drivers/block/xen-blkfront.c | 23 +++++++++++++++++++----
> 2 files changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
> index 8bfd1bc..25463e3 100644
> --- a/drivers/block/xen-blkback/xenbus.c
> +++ b/drivers/block/xen-blkback/xenbus.c
> @@ -704,6 +704,11 @@ again:
> dev->nodename);
> goto abort;
> }
> + err = xenbus_printf(xbt, dev->nodename, "physical-sector-size", "%u",
> + bdev_physical_block_size(be->blkif->vbd.bdev));
> + if (err)
> + xenbus_dev_error(dev, err, "writing %s/physical-sector-size",
> + dev->nodename);
>
> err = xenbus_transaction_end(xbt, 0);
> if (err == -EAGAIN)
> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> index d89ef86..33b5618 100644
> --- a/drivers/block/xen-blkfront.c
> +++ b/drivers/block/xen-blkfront.c
> @@ -542,7 +542,8 @@ wait:
> flush_requests(info);
> }
>
> -static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
> +static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size,
> + unsigned int physical_sector_size)
> {
> struct request_queue *rq;
> struct blkfront_info *info = gd->private_data;
> @@ -564,6 +565,7 @@ static int xlvbd_init_blk_queue(struct gendisk *gd, u16 sector_size)
>
> /* Hard sector size and max sectors impersonate the equiv. hardware. */
> blk_queue_logical_block_size(rq, sector_size);
> + blk_queue_physical_block_size(rq, physical_sector_size);
> blk_queue_max_hw_sectors(rq, 512);
>
> /* Each segment in a request is up to an aligned page in size. */
> @@ -667,7 +669,8 @@ static char *encode_disk_name(char *ptr, unsigned int n)
>
> static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
> struct blkfront_info *info,
> - u16 vdisk_info, u16 sector_size)
> + u16 vdisk_info, u16 sector_size,
> + unsigned int physical_sector_size)
> {
> struct gendisk *gd;
> int nr_minors = 1;
> @@ -734,7 +737,7 @@ static int xlvbd_alloc_gendisk(blkif_sector_t capacity,
> gd->driverfs_dev = &(info->xbdev->dev);
> set_capacity(gd, capacity);
>
> - if (xlvbd_init_blk_queue(gd, sector_size)) {
> + if (xlvbd_init_blk_queue(gd, sector_size, physical_sector_size)) {
> del_gendisk(gd);
> goto release;
> }
> @@ -1395,6 +1398,7 @@ static void blkfront_connect(struct blkfront_info *info)
> {
> unsigned long long sectors;
> unsigned long sector_size;
> + unsigned int physical_sector_size;
> unsigned int binfo;
> int err;
> int barrier, flush, discard, persistent;
> @@ -1437,6 +1441,16 @@ static void blkfront_connect(struct blkfront_info *info)
> return;
> }
>
> + /*
> + * physcial-sector-size is a newer field, so old backends may not
> + * provide this. Assume physical sector size to be the same as
> + * sector_size in that case.
> + */
> + err = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
> + "physical-sector-size", "%u", &physical_sector_size);
> + if (err != 1)
> + physical_sector_size = sector_size;
> +
> info->feature_flush = 0;
> info->flush_op = 0;
>
> @@ -1483,7 +1497,8 @@ static void blkfront_connect(struct blkfront_info *info)
> else
> info->feature_persistent = persistent;
>
> - err = xlvbd_alloc_gendisk(sectors, info, binfo, sector_size);
> + err = xlvbd_alloc_gendisk(sectors, info, binfo, sector_size,
> + physical_sector_size);
> if (err) {
> xenbus_dev_fatal(info->xbdev, err, "xlvbd_add at %s",
> info->xbdev->otherend);
> --
> 1.7.9.5
>
next prev parent reply other threads:[~2013-05-28 12:47 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-05-13 17:47 [PATCH] xen-blk(front|back): Handle large physical sector disks Stefan Bader
2013-05-14 8:04 ` Jan Beulich
2013-05-15 9:26 ` Stefan Bader
2013-05-15 9:47 ` Jan Beulich
2013-05-15 10:04 ` James Harper
2013-05-15 10:10 ` Stefan Bader
2013-05-15 12:23 ` Jan Beulich
2013-05-15 13:54 ` Konrad Rzeszutek Wilk
2013-05-15 10:58 ` Stefan Bader
2013-05-15 12:28 ` Jan Beulich
2013-05-14 14:49 ` Roger Pau Monné
2013-05-14 16:11 ` Stefan Bader
2013-05-15 15:02 ` [PATCH v3] " Stefan Bader
2013-05-22 11:48 ` Stefan Bader
2013-05-22 12:21 ` Jan Beulich
2013-05-22 13:15 ` Stefan Bader
2013-05-22 13:22 ` Jan Beulich
2013-05-22 13:02 ` Konrad Rzeszutek Wilk
2013-05-28 12:47 ` Konrad Rzeszutek Wilk [this message]
2013-05-28 12:55 ` Stefan Bader
2013-05-28 16:17 ` Konrad Rzeszutek Wilk
2013-05-28 18:03 ` Stefan Bader
2013-06-05 20:25 ` Konrad Rzeszutek Wilk
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130528124756.GA3754@phenom.dumpdata.com \
--to=konrad.wilk@oracle.com \
--cc=jbeulich@suse.com \
--cc=roger.pau@citrix.com \
--cc=stefan.bader@canonical.com \
--cc=xen-devel@lists.xensource.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.