xen-block: scale sector based quantities correctly
diff mbox series

Message ID 20190320162633.29479-1-paul.durrant@citrix.com
State New
Headers show
Series
  • xen-block: scale sector based quantities correctly
Related show

Commit Message

Paul Durrant March 20, 2019, 4:26 p.m. UTC
The mail thread at [1] clarifies that the Xen blkif protocol requires that
sector based quantities should be interpreted strictly as multiples of
512 bytes. This patch modifies the xen-block code accordingly.

[1] https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg01600.html

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
---
 hw/block/dataplane/xen-block.c | 28 +++++++++++++---------------
 hw/block/xen-block.c           |  2 +-
 hw/block/xen_blkif.h           |  2 ++
 3 files changed, 16 insertions(+), 16 deletions(-)

Comments

Anthony PERARD March 21, 2019, 3 p.m. UTC | #1
On Wed, Mar 20, 2019 at 04:26:32PM +0000, Paul Durrant wrote:
> The mail thread at [1] clarifies that the Xen blkif protocol requires that
> sector based quantities should be interpreted strictly as multiples of
> 512 bytes. This patch modifies the xen-block code accordingly.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg01600.html

So, that patch [1] only makes one change: how to interpret the "sectors"
node. It doesn't change the unit of a sector in blkif_request and
blkif_request_segment.

For a request, there is a comment in blkif.h:
    NB. first_sect and last_sect in blkif_request_segment, as well as
    sector_number in blkif_request, are always expressed in 512-byte units.

I think most of this QEMU patch is to comply with that comment and would
be a bug fix, but the single line change in hw/block/xen-block.c is
different.

> diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> index a848849f48..57e9da7e1c 100644
> --- a/hw/block/xen-block.c
> +++ b/hw/block/xen-block.c
> @@ -149,7 +149,7 @@ static void xen_block_set_size(XenBlockDevice *blockdev)
>      const char *type = object_get_typename(OBJECT(blockdev));
>      XenBlockVdev *vdev = &blockdev->props.vdev;
>      BlockConf *conf = &blockdev->props.conf;
> -    int64_t sectors = blk_getlength(conf->blk) / conf->logical_block_size;
> +    int64_t sectors = blk_getlength(conf->blk) / XEN_BLKIF_SECTOR_SIZE;

That the only thing that the thread [1] is changing.

>      XenDevice *xendev = XEN_DEVICE(blockdev);
>  
>      trace_xen_block_size(type, vdev->disk, vdev->partition, sectors);

Thanks,
Paul Durrant March 21, 2019, 3:02 p.m. UTC | #2
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 21 March 2019 15:00
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: xen-devel@lists.xenproject.org; qemu-block@nongnu.org; qemu-devel@nongnu.org; Stefan Hajnoczi
> <stefanha@redhat.com>; Stefano Stabellini <sstabellini@kernel.org>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>
> Subject: Re: [PATCH] xen-block: scale sector based quantities correctly
> 
> On Wed, Mar 20, 2019 at 04:26:32PM +0000, Paul Durrant wrote:
> > The mail thread at [1] clarifies that the Xen blkif protocol requires that
> > sector based quantities should be interpreted strictly as multiples of
> > 512 bytes. This patch modifies the xen-block code accordingly.
> >
> > [1] https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg01600.html
> 
> So, that patch [1] only makes one change: how to interpret the "sectors"
> node. It doesn't change the unit of a sector in blkif_request and
> blkif_request_segment.
> 
> For a request, there is a comment in blkif.h:
>     NB. first_sect and last_sect in blkif_request_segment, as well as
>     sector_number in blkif_request, are always expressed in 512-byte units.
> 
> I think most of this QEMU patch is to comply with that comment and would
> be a bug fix, but the single line change in hw/block/xen-block.c is
> different.
> 
> > diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
> > index a848849f48..57e9da7e1c 100644
> > --- a/hw/block/xen-block.c
> > +++ b/hw/block/xen-block.c
> > @@ -149,7 +149,7 @@ static void xen_block_set_size(XenBlockDevice *blockdev)
> >      const char *type = object_get_typename(OBJECT(blockdev));
> >      XenBlockVdev *vdev = &blockdev->props.vdev;
> >      BlockConf *conf = &blockdev->props.conf;
> > -    int64_t sectors = blk_getlength(conf->blk) / conf->logical_block_size;
> > +    int64_t sectors = blk_getlength(conf->blk) / XEN_BLKIF_SECTOR_SIZE;
> 
> That the only thing that the thread [1] is changing.
> 

Ok, true. I'll split this up into two patches then.

  Paul

> >      XenDevice *xendev = XEN_DEVICE(blockdev);
> >
> >      trace_xen_block_size(type, vdev->disk, vdev->partition, sectors);
> 
> Thanks,
> 
> --
> Anthony PERARD

Patch
diff mbox series

diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index f1523c5b45..bb8f1186e4 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -49,7 +49,6 @@  struct XenBlockDataPlane {
     unsigned int *ring_ref;
     unsigned int nr_ring_ref;
     void *sring;
-    int64_t file_blk;
     int protocol;
     blkif_back_rings_t rings;
     int more_work;
@@ -168,7 +167,7 @@  static int xen_block_parse_request(XenBlockRequest *request)
         goto err;
     }
 
-    request->start = request->req.sector_number * dataplane->file_blk;
+    request->start = request->req.sector_number * XEN_BLKIF_SECTOR_SIZE;
     for (i = 0; i < request->req.nr_segments; i++) {
         if (i == BLKIF_MAX_SEGMENTS_PER_REQUEST) {
             error_report("error: nr_segments too big");
@@ -178,14 +177,14 @@  static int xen_block_parse_request(XenBlockRequest *request)
             error_report("error: first > last sector");
             goto err;
         }
-        if (request->req.seg[i].last_sect * dataplane->file_blk >=
+        if (request->req.seg[i].last_sect * XEN_BLKIF_SECTOR_SIZE >=
             XC_PAGE_SIZE) {
             error_report("error: page crossing");
             goto err;
         }
 
         len = (request->req.seg[i].last_sect -
-               request->req.seg[i].first_sect + 1) * dataplane->file_blk;
+               request->req.seg[i].first_sect + 1) * XEN_BLKIF_SECTOR_SIZE;
         request->size += len;
     }
     if (request->start + request->size > blk_getlength(dataplane->blk)) {
@@ -205,7 +204,6 @@  static int xen_block_copy_request(XenBlockRequest *request)
     XenDevice *xendev = dataplane->xendev;
     XenDeviceGrantCopySegment segs[BLKIF_MAX_SEGMENTS_PER_REQUEST];
     int i, count;
-    int64_t file_blk = dataplane->file_blk;
     bool to_domain = (request->req.operation == BLKIF_OP_READ);
     void *virt = request->buf;
     Error *local_err = NULL;
@@ -220,16 +218,17 @@  static int xen_block_copy_request(XenBlockRequest *request)
         if (to_domain) {
             segs[i].dest.foreign.ref = request->req.seg[i].gref;
             segs[i].dest.foreign.offset = request->req.seg[i].first_sect *
-                file_blk;
+                XEN_BLKIF_SECTOR_SIZE;
             segs[i].source.virt = virt;
         } else {
             segs[i].source.foreign.ref = request->req.seg[i].gref;
             segs[i].source.foreign.offset = request->req.seg[i].first_sect *
-                file_blk;
+                XEN_BLKIF_SECTOR_SIZE;
             segs[i].dest.virt = virt;
         }
         segs[i].len = (request->req.seg[i].last_sect -
-                       request->req.seg[i].first_sect + 1) * file_blk;
+                       request->req.seg[i].first_sect + 1) *
+                      XEN_BLKIF_SECTOR_SIZE;
         virt += segs[i].len;
     }
 
@@ -331,22 +330,22 @@  static bool xen_block_split_discard(XenBlockRequest *request,
     XenBlockDataPlane *dataplane = request->dataplane;
     int64_t byte_offset;
     int byte_chunk;
-    uint64_t byte_remaining, limit;
+    uint64_t byte_remaining;
     uint64_t sec_start = sector_number;
     uint64_t sec_count = nr_sectors;
 
     /* Wrap around, or overflowing byte limit? */
     if (sec_start + sec_count < sec_count ||
-        sec_start + sec_count > INT64_MAX / dataplane->file_blk) {
+        sec_start + sec_count > INT64_MAX / XEN_BLKIF_SECTOR_SIZE) {
         return false;
     }
 
-    limit = BDRV_REQUEST_MAX_SECTORS * dataplane->file_blk;
-    byte_offset = sec_start * dataplane->file_blk;
-    byte_remaining = sec_count * dataplane->file_blk;
+    byte_offset = sec_start * XEN_BLKIF_SECTOR_SIZE;
+    byte_remaining = sec_count * XEN_BLKIF_SECTOR_SIZE;
 
     do {
-        byte_chunk = byte_remaining > limit ? limit : byte_remaining;
+        byte_chunk = byte_remaining > BDRV_REQUEST_MAX_BYTES ?
+            BDRV_REQUEST_MAX_BYTES : byte_remaining;
         request->aio_inflight++;
         blk_aio_pdiscard(dataplane->blk, byte_offset, byte_chunk,
                          xen_block_complete_aio, request);
@@ -632,7 +631,6 @@  XenBlockDataPlane *xen_block_dataplane_create(XenDevice *xendev,
     XenBlockDataPlane *dataplane = g_new0(XenBlockDataPlane, 1);
 
     dataplane->xendev = xendev;
-    dataplane->file_blk = conf->logical_block_size;
     dataplane->blk = conf->blk;
 
     QLIST_INIT(&dataplane->inflight);
diff --git a/hw/block/xen-block.c b/hw/block/xen-block.c
index a848849f48..57e9da7e1c 100644
--- a/hw/block/xen-block.c
+++ b/hw/block/xen-block.c
@@ -149,7 +149,7 @@  static void xen_block_set_size(XenBlockDevice *blockdev)
     const char *type = object_get_typename(OBJECT(blockdev));
     XenBlockVdev *vdev = &blockdev->props.vdev;
     BlockConf *conf = &blockdev->props.conf;
-    int64_t sectors = blk_getlength(conf->blk) / conf->logical_block_size;
+    int64_t sectors = blk_getlength(conf->blk) / XEN_BLKIF_SECTOR_SIZE;
     XenDevice *xendev = XEN_DEVICE(blockdev);
 
     trace_xen_block_size(type, vdev->disk, vdev->partition, sectors);
diff --git a/hw/block/xen_blkif.h b/hw/block/xen_blkif.h
index 3e6e1ea365..a353693ea0 100644
--- a/hw/block/xen_blkif.h
+++ b/hw/block/xen_blkif.h
@@ -143,4 +143,6 @@  static inline void blkif_get_x86_64_req(blkif_request_t *dst,
     }
 }
 
+#define XEN_BLKIF_SECTOR_SIZE 512
+
 #endif /* XEN_BLKIF_H */