Xen devel - May 2009 - [PATCH 1 of 2] blkfront: sector size > 512

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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).   */

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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).   */

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel