fs/bio.c - Hardcoded sector size ?

fs/bio.c - Hardcoded sector size ?

[Roger Gammans]

Hi

In bio_endio()  there is the follow line:- 

	bio->bi_sector += (bytes_done >> 9);

and there is a similiar line assuming a 512byte sector in 
__bio_add_page() .

Is this a bug as the the actual  block size  should be available
from bio->bi_bdev->bd_block_size surely - or is some clever code
where all block devices have to translate back to 512 byte sectors
for bio s.

TTFN
-- 
Roger.                          Home| http://www.sandman.uklinux.net/
Master of Peng Shui.      (Ancient oriental art of Penguin Arranging)
Work|Independent Sys Consultant | http://www.computer-surgery.co.uk/
So what are the eigenvalues and eigenvectors of 'The Matrix'? --anon

Re: fs/bio.c - Hardcoded sector size ?

[linux-kernel-owner]

On Fri, Sep 29, 2006 at 11:38:14AM -0700, Randy Dunlap wrote:
> > from bio->bi_bdev->bd_block_size surely - or is some clever code
> > where all block devices have to translate back to 512 byte sectors

> Does this answer it for you?

Ahh, Yup. 

Is this documented anywhere ? , I'd suggest a <para> in kernel-api.tmpl
but I'm not sure this is the right place.

TTFN
-- 
Roger.                          Home| http://www.sandman.uklinux.net/
Master of Peng Shui.      (Ancient oriental art of Penguin Arranging)
Work|Independent Sys Consultant | http://www.computer-surgery.co.uk/
So what are the eigenvalues and eigenvectors of 'The Matrix'? --anon

Re: fs/bio.c - Hardcoded sector size ?

[Roger Gammans]

[-- Attachment #1: Type: text/plain, Size: 784 bytes --]

On Fri, Sep 29, 2006 at 12:11:57PM -0700, Randy Dunlap wrote:
> I don't know if or where it is documented.

Well, I've spend a good chunk of time reading round this part of
the kernel's interfaces without spotting it so another note somewhere
can't help.

> You can submit a patch for it.
> If you don't, I'll put it in my todo queue.

If I find an approriate place to put such a note I'll add it and
submit a patch, but I'm not sure where to put it , atm.

Any suggestions?

TTFN
-- 
Roger.                          Home| http://www.sandman.uklinux.net/
Master of Peng Shui.      (Ancient oriental art of Penguin Arranging)
Work|Independent Sys Consultant | http://www.computer-surgery.co.uk/
So what are the eigenvalues and eigenvectors of 'The Matrix'? --anon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: fs/bio.c - Hardcoded sector size ?

[Roger Gammans]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

On Fri, Sep 29, 2006 at 12:37:37PM -0700, Randy Dunlap wrote:
> Hm, I looked thru fs/bio.c and block/*.c and Documentation/Docbook/*.tmpl.
> The best place that I see to put it right now is in
> include/linux/bio.h, struct bio, field: bi_sector.
> 
> What do you think of that?

Well, ... Um. I can't think of anywhere better either, so how about
this:-

Signed-Off-By: Roger Gammans <rgammans@computer-sugery.co.uk>

diff --git a/include/linux/bio.h b/include/linux/bio.h
index 76bdaea..77a8e6b 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -70,7 +70,8 @@ typedef void (bio_destructor_t) (struct
  * stacking drivers)
  */
 struct bio {
-       sector_t                bi_sector;
+       sector_t                bi_sector;      /* device address in 512 byte
+                                                  sectors */
        struct bio              *bi_next;       /* request queue link */
        struct block_device     *bi_bdev;
        unsigned long           bi_flags;       /* status, command, etc
*/


-- 
Roger.                          Home| http://www.sandman.uklinux.net/
Master of Peng Shui.      (Ancient oriental art of Penguin Arranging)
Work|Independent Sys Consultant | http://www.computer-surgery.co.uk/
So what are the eigenvalues and eigenvectors of 'The Matrix'? --anon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: fs/bio.c - Hardcoded sector size ?

[Roger Gammans]

[-- Attachment #1: Type: text/plain, Size: 2089 bytes --]

On Fri, Sep 29, 2006 at 01:32:05PM -0700, Randy Dunlap wrote:
> > How about adding kerneldoc for sector_t itself?
> 
> Good idea, but afaik it would have to be added for the entire
> struct, not just one field.

sector_t 's a simple typedef from unsigned long or u64 depending on
config rather than a struct - will kerneldoc still pick up the comments
on theese?

Assuming it will I suggest the following. I've kept my shorter text in
the bi_sector field as it is now more fully explained with sector_t.


Signed-Off-By: Roger Gammans <rgammans@computer-surgery.co.uk>

diff --git a/include/linux/bio.h b/include/linux/bio.h
index 76bdaea..77a8e6b 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -70,7 +70,8 @@ typedef void (bio_destructor_t) (struct
  * stacking drivers)
  */
 struct bio {
-       sector_t                bi_sector;
+       sector_t                bi_sector;      /* device address in 512 byte
+                                                  sectors */
        struct bio              *bi_next;       /* request queue link */
        struct block_device     *bi_bdev;
        unsigned long           bi_flags;       /* status, command, etc
*/
diff --git a/include/linux/types.h b/include/linux/types.h
index 3f23566..0ddfa1a 100644
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -127,8 +127,12 @@ #endif
 /* this is a special 64bit data type that is 8-byte aligned */
 #define aligned_u64 unsigned long long __attribute__((aligned(8)))

-/*
+/**
  * The type used for indexing onto a disc or disc partition.
+ *
+ * Linux always considers sectors to be 512 bytes long independently
+ * of the devices real block size.
+ *
  * If required, asm/types.h can override it and define
  * HAVE_SECTOR_T
  */


-- 
Roger.                          Home| http://www.sandman.uklinux.net/
Master of Peng Shui.      (Ancient oriental art of Penguin Arranging)
Work|Independent Sys Consultant | http://www.computer-surgery.co.uk/
So what are the eigenvalues and eigenvectors of 'The Matrix'? --anon

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: fs/bio.c - Hardcoded sector size ?

[Randy Dunlap]

On Thu, 28 Sep 2006 19:22:38 +0100 Roger Gammans wrote:

> Hi
> 
> In bio_endio()  there is the follow line:- 
> 
> 	bio->bi_sector += (bytes_done >> 9);
> 
> and there is a similiar line assuming a 512byte sector in 
> __bio_add_page() .
> 
> Is this a bug as the the actual  block size  should be available
> from bio->bi_bdev->bd_block_size surely - or is some clever code
> where all block devices have to translate back to 512 byte sectors
> for bio s.

Does this answer it for you?

http://marc.theaimsgroup.com/?l=linux-kernel&m=115542872706154&w=2

---
~Randy

Re: fs/bio.c - Hardcoded sector size ?

[Randy Dunlap]

On Thu, 28 Sep 2006 19:58:21 +0100 linux-kernel-owner@vger.kernel.org wrote:

> On Fri, Sep 29, 2006 at 11:38:14AM -0700, Randy Dunlap wrote:
> > > from bio->bi_bdev->bd_block_size surely - or is some clever code
> > > where all block devices have to translate back to 512 byte sectors
> 
> > Does this answer it for you?
> 
> Ahh, Yup. 
> 
> Is this documented anywhere ? , I'd suggest a <para> in kernel-api.tmpl
> but I'm not sure this is the right place.

I don't know if or where it is documented.
You can submit a patch for it.
If you don't, I'll put it in my todo queue.

---
~Randy

Re: fs/bio.c - Hardcoded sector size ?

[Randy Dunlap]

On Thu, 28 Sep 2006 20:19:46 +0100 Roger Gammans wrote:

> On Fri, Sep 29, 2006 at 12:11:57PM -0700, Randy Dunlap wrote:
> > I don't know if or where it is documented.
> 
> Well, I've spend a good chunk of time reading round this part of
> the kernel's interfaces without spotting it so another note somewhere
> can't help.
> 
> > You can submit a patch for it.
> > If you don't, I'll put it in my todo queue.
> 
> If I find an approriate place to put such a note I'll add it and
> submit a patch, but I'm not sure where to put it , atm.
> 
> Any suggestions?

Hm, I looked thru fs/bio.c and block/*.c and Documentation/Docbook/*.tmpl.
The best place that I see to put it right now is in
include/linux/bio.h, struct bio, field: bi_sector.

What do you think of that?
---
~Randy
GPL v0:  http://www.glacierparkinc.com/GlacierParkLodge.htm

Re: fs/bio.c - Hardcoded sector size ?

[Randy Dunlap]

On Thu, 28 Sep 2006 20:56:27 +0100 Roger Gammans wrote:

> On Fri, Sep 29, 2006 at 12:37:37PM -0700, Randy Dunlap wrote:
> > Hm, I looked thru fs/bio.c and block/*.c and Documentation/Docbook/*.tmpl.
> > The best place that I see to put it right now is in
> > include/linux/bio.h, struct bio, field: bi_sector.
> > 
> > What do you think of that?
> 
> Well, ... Um. I can't think of anywhere better either, so how about
> this:-
> 
> Signed-Off-By: Roger Gammans <rgammans@computer-sugery.co.uk>


Looks OK to me.  I would probably go for something a little
stronger, though, like:

	sector_t		bi_sector;	/* block layer sector
						 * addresses are always in
						 * 512-byte units in Linux */

Jens, is something like this (above or below) OK with you?


> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 76bdaea..77a8e6b 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -70,7 +70,8 @@ typedef void (bio_destructor_t) (struct
>   * stacking drivers)
>   */
>  struct bio {
> -       sector_t                bi_sector;
> +       sector_t                bi_sector;      /* device address in 512 byte
> +                                                  sectors */
>         struct bio              *bi_next;       /* request queue link */
>         struct block_device     *bi_bdev;
>         unsigned long           bi_flags;       /* status, command, etc
> */


---
~Randy

Re: fs/bio.c - Hardcoded sector size ?

[Zach Brown]

> 	sector_t		bi_sector;	/* block layer sector
> 						 * addresses are always in
> 						 * 512-byte units in Linux */

How about adding kerneldoc for sector_t itself?

- z

Re: fs/bio.c - Hardcoded sector size ?

[Randy Dunlap]

On Fri, 29 Sep 2006 13:22:34 -0700 Zach Brown wrote:

> 
> > 	sector_t		bi_sector;	/* block layer sector
> > 						 * addresses are always in
> > 						 * 512-byte units in Linux */
> 
> How about adding kerneldoc for sector_t itself?

Good idea, but afaik it would have to be added for the entire
struct, not just one field.

---
~Randy

Re: fs/bio.c - Hardcoded sector size ?

[Jens Axboe]

On Fri, Sep 29 2006, Roger Gammans wrote:
> On Fri, Sep 29, 2006 at 01:32:05PM -0700, Randy Dunlap wrote:
> > > How about adding kerneldoc for sector_t itself?
> > 
> > Good idea, but afaik it would have to be added for the entire
> > struct, not just one field.
> 
> sector_t 's a simple typedef from unsigned long or u64 depending on
> config rather than a struct - will kerneldoc still pick up the comments
> on theese?
> 
> Assuming it will I suggest the following. I've kept my shorter text in
> the bi_sector field as it is now more fully explained with sector_t.
> 
> 
> Signed-Off-By: Roger Gammans <rgammans@computer-surgery.co.uk>
> 
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 76bdaea..77a8e6b 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -70,7 +70,8 @@ typedef void (bio_destructor_t) (struct
>   * stacking drivers)
>   */
>  struct bio {
> -       sector_t                bi_sector;
> +       sector_t                bi_sector;      /* device address in 512 byte
> +                                                  sectors */
>         struct bio              *bi_next;       /* request queue link */
>         struct block_device     *bi_bdev;
>         unsigned long           bi_flags;       /* status, command, etc
> */
> diff --git a/include/linux/types.h b/include/linux/types.h
> index 3f23566..0ddfa1a 100644
> --- a/include/linux/types.h
> +++ b/include/linux/types.h
> @@ -127,8 +127,12 @@ #endif
>  /* this is a special 64bit data type that is 8-byte aligned */
>  #define aligned_u64 unsigned long long __attribute__((aligned(8)))
> 
> -/*
> +/**
>   * The type used for indexing onto a disc or disc partition.
> + *
> + * Linux always considers sectors to be 512 bytes long independently
> + * of the devices real block size.
> + *
>   * If required, asm/types.h can override it and define
>   * HAVE_SECTOR_T
>   */

Looks fine to me, I'll add it (although I tend to prefer disk :-)

-- 
Jens Axboe