[PATCH 1 of 2] blkfront: sector size > 512

[PATCH 1 of 2] blkfront: sector size > 512

[Stefano Stabellini]

The first and last sector as well as the sector number of the request is
expressed in 512 bytes units, independently from the real sector size.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> 

---

diff -r 61404d971c92 extras/mini-os/blkfront.c
--- a/extras/mini-os/blkfront.c	Thu May 21 04:31:47 2009 +0100
+++ b/extras/mini-os/blkfront.c	Thu May 21 11:50:56 2009 +0100
@@ -310,14 +310,14 @@
     req->nr_segments = n;
     req->handle = dev->handle;
     req->id = (uintptr_t) aiocbp;
-    req->sector_number = aiocbp->aio_offset / dev->info.sector_size;
+    req->sector_number = aiocbp->aio_offset / 512;
 
     for (j = 0; j < n; j++) {
         req->seg[j].first_sect = 0;
-        req->seg[j].last_sect = PAGE_SIZE / dev->info.sector_size - 1;
+        req->seg[j].last_sect = PAGE_SIZE / 512 - 1;
     }
-    req->seg[0].first_sect = ((uintptr_t)aiocbp->aio_buf & ~PAGE_MASK) / dev->info.sector_size;
-    req->seg[n-1].last_sect = (((uintptr_t)aiocbp->aio_buf + aiocbp->aio_nbytes - 1) & ~PAGE_MASK) / dev->info.sector_size;
+    req->seg[0].first_sect = ((uintptr_t)aiocbp->aio_buf & ~PAGE_MASK) / 512;
+    req->seg[n-1].last_sect = (((uintptr_t)aiocbp->aio_buf + aiocbp->aio_nbytes - 1) & ~PAGE_MASK) / 512;
     for (j = 0; j < n; j++) {
 	uintptr_t data = start + j * PAGE_SIZE;
         if (!write) {

Re: [PATCH 1 of 2] blkfront: sector size > 512

[Samuel Thibault]

Stefano Stabellini, le Thu 21 May 2009 12:40:52 +0100, a écrit :
> The first and last sector as well as the sector number of the request is
> expressed in 512 bytes units, independently from the real sector size.
>      req->id = (uintptr_t) aiocbp;
> -    req->sector_number = aiocbp->aio_offset / dev->info.sector_size;
> +    req->sector_number = aiocbp->aio_offset / 512;
>  

Oh?!

That needs to be better documented in xen/include/public/io/blkif.h,
then.  I'm however wondering whether this shouldn't actually be fixed in
the blkback/cdrom interface instead.

Samuel

Re: [PATCH 1 of 2] blkfront: sector size > 512

[Stefano Stabellini]

Samuel Thibault wrote:

> Stefano Stabellini, le Thu 21 May 2009 12:40:52 +0100, a écrit :
>> The first and last sector as well as the sector number of the request is
>> expressed in 512 bytes units, independently from the real sector size.
>>      req->id = (uintptr_t) aiocbp;
>> -    req->sector_number = aiocbp->aio_offset / dev->info.sector_size;
>> +    req->sector_number = aiocbp->aio_offset / 512;
>>  
> 
> Oh?!
> 
> That needs to be better documented in xen/include/public/io/blkif.h,
> then.  I'm however wondering whether this shouldn't actually be fixed in
> the blkback/cdrom interface instead.
> 

Yeah, I am surprised as you are, but the linux blkfront does the same
thing so I prefer to avoid changing the protocol now.

Re: [PATCH 1 of 2] blkfront: sector size > 512

[Samuel Thibault]

Stefano Stabellini, le Thu 21 May 2009 14:17:11 +0100, a écrit :
> Samuel Thibault wrote:
> > Stefano Stabellini, le Thu 21 May 2009 12:40:52 +0100, a écrit :
> >> The first and last sector as well as the sector number of the request is
> >> expressed in 512 bytes units, independently from the real sector size.
> >>      req->id = (uintptr_t) aiocbp;
> >> -    req->sector_number = aiocbp->aio_offset / dev->info.sector_size;
> >> +    req->sector_number = aiocbp->aio_offset / 512;
> >>  
> > 
> > Oh?!
> > 
> > That needs to be better documented in xen/include/public/io/blkif.h,
> > then.  I'm however wondering whether this shouldn't actually be fixed in
> > the blkback/cdrom interface instead.
> > 
> 
> Yeah, I am surprised as you are, but the linux blkfront does the same
> thing so I prefer to avoid changing the protocol now.

Well, it's not about changing the protocol, it's about fixing drivers
doing the wrong thing according to the documentation.  What is the use
of the xenstore sector_size value if not here?

Samuel

Re: [PATCH 1 of 2] blkfront: sector size > 512

[Keir Fraser]

On 21/05/2009 06:20, "Samuel Thibault" <samuel.thibault@ens-lyon.org> wrote:

>> Yeah, I am surprised as you are, but the linux blkfront does the same
>> thing so I prefer to avoid changing the protocol now.
> 
> Well, it's not about changing the protocol, it's about fixing drivers
> doing the wrong thing according to the documentation.  What is the use
> of the xenstore sector_size value if not here?

It's due to the protocol having evolved a bit. The xenstore sector_size was
added a bit later, specifically after testing on CD-ROM drives, to require
that frontends issue sector_size-aligned and -sized requests. Otherwise
Linux blkdev subsystem was dropping the reqs on the floor. But we did not
change the wire request format at that time.

Yeah, I'd take a patch to add a nice clear comment about this to blkif.h.
;-)

 -- Keir

Re: [PATCH 1 of 2] blkfront: sector size > 512

[Stefano Stabellini]

Keir Fraser wrote:

> 
> Yeah, I'd take a patch to add a nice clear comment about this to blkif.h.
> ;-)
> 



Is this comment clear enough?

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

---


diff -r 61404d971c92 xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Thu May 21 04:31:47 2009 +0100
+++ b/xen/include/public/io/blkif.h	Fri May 22 15:29:19 2009 +0100
@@ -84,6 +84,13 @@
  */
 #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
 
+/* 
+ * first_sect and last_sect in blkif_request_segment, as well as
+ * sector_number in blkif_request are all expressed in 512 bytes units,
+ * however they must be properly aligned to the real sector size of
+ * the physical disk (as reported in the "sector-size" node in backend
+ * xenbus info).
+ */
 struct blkif_request_segment {
     grant_ref_t gref;        /* reference to I/O buffer frame        */
     /* @first_sect: first sector in frame to transfer (inclusive).   */

Re: [PATCH 1 of 2] blkfront: sector size > 512

[Samuel Thibault]

Stefano Stabellini, le Fri 22 May 2009 15:31:28 +0100, a écrit :
> Keir Fraser wrote:
> > Yeah, I'd take a patch to add a nice clear comment about this to blkif.h.
> > ;-)
> 
> Is this comment clear enough?

Could you also document whether the 'sectors' node in the xenstore is
expressed in sector size or in 512 sectors?

Samuel

Re: [PATCH 1 of 2] blkfront: sector size > 512

[Stefano Stabellini]

Samuel Thibault wrote:

> Stefano Stabellini, le Fri 22 May 2009 15:31:28 +0100, a écrit :
>> Keir Fraser wrote:
>>> Yeah, I'd take a patch to add a nice clear comment about this to blkif.h.
>>> ;-)
>> Is this comment clear enough?
> 
> Could you also document whether the 'sectors' node in the xenstore is
> expressed in sector size or in 512 sectors?



OK.

---

diff -r 61404d971c92 xen/include/public/io/blkif.h
--- a/xen/include/public/io/blkif.h	Thu May 21 04:31:47 2009 +0100
+++ b/xen/include/public/io/blkif.h	Fri May 22 15:54:29 2009 +0100
@@ -84,6 +84,14 @@
  */
 #define BLKIF_MAX_SEGMENTS_PER_REQUEST 11
 
+/* 
+ * first_sect and last_sect in blkif_request_segment, as well as
+ * sector_number in blkif_request are all expressed in 512 bytes units,
+ * however they must be properly aligned to the real sector size of
+ * the physical disk, that is reported in the "sector-size" node in
+ * backend xenbus info.
+ * Also the "sectors" node on xenbus is expressed in 512 bytes units.
+ */
 struct blkif_request_segment {
     grant_ref_t gref;        /* reference to I/O buffer frame        */
     /* @first_sect: first sector in frame to transfer (inclusive).   */