BLKGETSIZE64 (bytes or sectors?)

BLKGETSIZE64 (bytes or sectors?)

[Matt Domsch]

Is the BLKGETSIZE64 ioctl supposed to return the size of the device in 
bytes (as the comment says, and is implemented in all places *except* 
blkpg.c), or in sectors (as is implemented in blkpg.c since 2.4.15)?

It would seem that blkpg.c gets it wrong, that it should be in bytes.  
Assuming that's the case, here's the patch to fix it against 2.4.18-pre4.

Thanks,
Matt

-- 
Matt Domsch
Sr. Software Engineer
Dell Linux Solutions www.dell.com/linux
#1 US Linux Server provider with 24.5% (IDC Dec 2001)
#2 Worldwide Linux Server provider with 18.2% (IDC Dec 2001)


--- linux-2.4.18-pre4/drivers/block/blkpg.c.orig	Thu Jan 17 14:24:24 2002
+++ linux-2.4.18-pre4/drivers/block/blkpg.c	Thu Jan 17 14:26:43 2002
@@ -247,7 +247,7 @@ int blk_ioctl(kdev_t dev, unsigned int c
 			if (cmd == BLKGETSIZE)
 				return put_user((unsigned long)ullval, (unsigned long *)arg);
 			else
-				return put_user(ullval, (u64 *)arg);
+				return put_user((u64)ullval << 9 , (u64 *)arg);
 #if 0
 		case BLKRRPART: /* Re-read partition tables */
 			if (!capable(CAP_SYS_ADMIN)) 

Re: BLKGETSIZE64 (bytes or sectors?)

[Tim Pepper]

On Thu 17 Jan at 14:28:52 -0600 Matt_Domsch@Dell.com done said:
> Is the BLKGETSIZE64 ioctl supposed to return the size of the device in 
> bytes (as the comment says, and is implemented in all places *except* 
> blkpg.c), or in sectors (as is implemented in blkpg.c since 2.4.15)?
> 
> It would seem that blkpg.c gets it wrong, that it should be in bytes.  
> Assuming that's the case, here's the patch to fix it against 2.4.18-pre4.

I was just in the process of writing a post for the same thing.  Wouldn't it
be better to do the following (against 2.4.17).

Tim

-- 
*********************************************************
*  tpepper@vato dot org             * Venimus, Vidimus, *
*  http://www.vato.org/~tpepper     * Dolavimus         *
*********************************************************


--- linux-2.4.17-orig/blkpg.c	Thu Jan 17 13:02:19 2002
+++ linux-2.4.17/blkpg.c	Thu Jan 17 13:06:33 2002
@@ -246,8 +246,14 @@
 
 			if (cmd == BLKGETSIZE)
 				return put_user((unsigned long)ullval, (unsigned long *)arg);
-			else
+			else {
+				if (hardsect_size[MAJOR(dev)][MINOR(dev)]) {
+					ullval *= hardsect_size[MAJOR(dev)][MINOR(dev)];
+				} else {
+					ullval *= 512;
+				}
 				return put_user(ullval, (u64 *)arg);
+			}
 #if 0
 		case BLKRRPART: /* Re-read partition tables */
 			if (!capable(CAP_SYS_ADMIN)) 

Re: BLKGETSIZE64 (bytes or sectors?)

[Benjamin LaHaise]

On Thu, Jan 17, 2002 at 02:28:52PM -0600, Matt Domsch wrote:
> Is the BLKGETSIZE64 ioctl supposed to return the size of the device in 
> bytes (as the comment says, and is implemented in all places *except* 
> blkpg.c), or in sectors (as is implemented in blkpg.c since 2.4.15)?
> 
> It would seem that blkpg.c gets it wrong, that it should be in bytes.  
> Assuming that's the case, here's the patch to fix it against 2.4.18-pre4.

blkpg.c is wrong -- the size has to be in bytes in order to support weird 
sector sizes.

		-ben

RE: BLKGETSIZE64 (bytes or sectors?)

[Matt_Domsch]

> Wouldn't it be better to do the following (against 2.4.17).

Yes, I agree.

--
Matt Domsch
Sr. Software Engineer
Dell Linux Solutions www.dell.com/linux
#1 US Linux Server provider with 24.5% (IDC Dec 2001)
#2 Worldwide Linux Server provider with 18.2% (IDC Dec 2001)

Re: BLKGETSIZE64 (bytes or sectors?)

[Tim Pepper]

On Thu 17 Jan at 16:34:50 -0600 Matt_Domsch@Dell.com done said:
> 
> Yes, I agree.

Alright, I'll wait a bit to see if anybody else comments and then bounce it to
Marcello.

Tim
-- 
*********************************************************
*  tpepper@vato dot org             * Venimus, Vidimus, *
*  http://www.vato.org/~tpepper     * Dolavimus         *
*********************************************************

Re: BLKGETSIZE64 (bytes or sectors?)

[Andries.Brouwer]

Matt_Domsch@dell.com wrote, and he is right:

    > Is the BLKGETSIZE64 ioctl supposed to return the size of the device in 
    > bytes (as the comment says, and is implemented in all places *except* 
    > blkpg.c), or in sectors (as is implemented in blkpg.c since 2.4.15)?

Yes, in bytes. blkpg.c has to be fixed.
Several people submitted patches. Sooner or later I suppose
this will be fixed.

Then Tim Pepper answered, and he is wrong:

    Wouldn't it be better to do the following (against 2.4.17).

    +            else {
    +                if (hardsect_size[MAJOR(dev)][MINOR(dev)]) {
    +                    ullval *= hardsect_size[MAJOR(dev)][MINOR(dev)];
    +                } else {
    +                    ullval *= 512;
    +                }
                     return put_user(ullval, (u64 *)arg);
    +            }

You see, the 512 here is 512, and has no relation to hardware
sector size. Multiplying with hardsect_size[][] is a bad bug.

Indeed, you can check this in fs/partitions/msdos.c, where
one reads
	int sector_size = get_hardsect_size(to_kdev_t(bdev->bd_dev)) / 512;
	...
	offs = START_SECT(p)*sector_size;
	size = NR_SECTS(p)*sector_size;
	...
	add_gd_partition(...);

So, indeed, we have already multiplied by hardsect_size, struct gendisk
uses sectors of size 512, independent of the hardware, and we must not
again multiply by hardsect_size.

Unfortunately Matt Domsch replied:

    > Yes, I agree.

but he meant: No!

Andries

Re: BLKGETSIZE64 (bytes or sectors?)

[Tim Pepper]

On Thu 17 Jan at 23:48:16 +0000 Andries.Brouwer@cwi.nl done said:
> Matt_Domsch@dell.com wrote, and he is right:
> 
> Yes, in bytes. blkpg.c has to be fixed.
> Several people submitted patches. Sooner or later I suppose
> this will be fixed.
> 
>--8< snip-----------
>
> So, indeed, we have already multiplied by hardsect_size, struct gendisk
> uses sectors of size 512, independent of the hardware, and we must not
> again multiply by hardsect_size.

Doh.  It's obviously much cleaner and more efficient that way.

Have any of the other patch submitters added a comment to note this in
include/linux/genhd.h?  hd_struct doesn't have any mention of start_sect
and nr_sect being stored in sectors of 512bytes.  But maybe that's just
to weed out fools like me.

-- 
*********************************************************
*  tpepper@vato dot org             * Venimus, Vidimus, *
*  http://www.vato.org/~tpepper     * Dolavimus         *
*********************************************************