Patchwork [PATCH/RFC] block: Ensure that block size constraints are considered

login
register
mail settings
Submitter Kevin Wolf
Date Dec. 11, 2012, 10:30 a.m.
Message ID <50C70B4D.1000505@redhat.com>
Download mbox | patch
Permalink /patch/205154/
State New
Headers show

Comments

Kevin Wolf - Dec. 11, 2012, 10:30 a.m.
Am 11.12.2012 10:58, schrieb Heinz Graalfs:
> Hi Kevin,
> 
> I'm using the bdrv_pread() function during boot partition detection ...
> 
> In detail: 
> bdrv_pread() is called to read 32 bytes from a 2048 bytes formatted
> disk. This results in setting up a read of 512 bytes (1 sector
> multiplied by 512 current code in paio_submit()), which is wrong for a
> O_DIRECT opened file, and produces the error.

So this sounds like the real problem: bdrv_pread/pwrite assume 512 byte
sectors. May it's better to fix it there instead of just fixing one code
path in one backend.

In any case this patch as submitted is wrong as it overflows the buffer
passed to paio_submit. Test it with this patch:

     return 0;


$ ./qemu-io -n -c 'read -p 0 512' /tmp/foo
read 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0001 sec (3.727 MiB/sec and 7633.5878 ops/sec)
*** glibc detected *** ./qemu-io: double free or corruption (out):
0x00007fa22349b000 ***

Kevin
Heinz Graalfs - Dec. 11, 2012, 1:53 p.m.
On Tue, 2012-12-11 at 11:30 +0100, Kevin Wolf wrote:
> Am 11.12.2012 10:58, schrieb Heinz Graalfs:
> > Hi Kevin,
> > 
> > I'm using the bdrv_pread() function during boot partition detection ...
> > 
> > In detail: 
> > bdrv_pread() is called to read 32 bytes from a 2048 bytes formatted
> > disk. This results in setting up a read of 512 bytes (1 sector
> > multiplied by 512 current code in paio_submit()), which is wrong for a
> > O_DIRECT opened file, and produces the error.
> 
> So this sounds like the real problem: bdrv_pread/pwrite assume 512 byte
> sectors. May it's better to fix it there instead of just fixing one code
> path in one backend.
> 
> In any case this patch as submitted is wrong as it overflows the buffer
> passed to paio_submit. Test it with this patch:
> 
> --- a/qemu-io.c
> +++ b/qemu-io.c
> @@ -1718,6 +1718,8 @@ static int openfile(char *name, int flags, int
> growable)
>              bs = NULL;
>              return 1;
>          }
> +
> +        bdrv_set_buffer_alignment(bs, 4096);
>      }
> 
>      return 0;
> 
> 
> $ ./qemu-io -n -c 'read -p 0 512' /tmp/foo
> read 512/512 bytes at offset 0
> 512 bytes, 1 ops; 0.0001 sec (3.727 MiB/sec and 7633.5878 ops/sec)
> *** glibc detected *** ./qemu-io: double free or corruption (out):
> 0x00007fa22349b000 ***
> 
> Kevin
> 

Kevin, I tried your fix and it solves the free error...

Here is what I get:

# lsdasd
Bus-ID     Status      Name      Device  Type  BlkSz  Size      Blocks
==============================================================================
0.0.37a1   active      dasdb     94:4    ECKD  2048   6162MB    3155355
0.0.37a0   active      dasdc     94:8    ECKD  512    3594MB    7362495

# ./qemu-io  -c 'read -p 0 512' /dev/disk/by-path/ccw-0.0.37a0
read 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0000 sec (7.512 MiB/sec and 15384.6154 ops/sec)

# ./qemu-io -n  -c 'read -p 0 512' /dev/disk/by-path/ccw-0.0.37a0
read 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0005 sec (904.159 KiB/sec and 1808.3183 ops/sec)


# ./qemu-io   -c 'read -p 0 512' /dev/disk/by-path/ccw-0.0.37a1
read 512/512 bytes at offset 0
512 bytes, 1 ops; 0.0000 sec (7.288 MiB/sec and 14925.3731 ops/sec)

# ./qemu-io -n  -c 'read -p 0 512' /dev/disk/by-path/ccw-0.0.37a1
read failed: Invalid argument
# 

Are you going to fix the rest in bdrv_pread/pwrite too?

Heinz

Patch

--- a/qemu-io.c
+++ b/qemu-io.c
@@ -1718,6 +1718,8 @@  static int openfile(char *name, int flags, int
growable)
             bs = NULL;
             return 1;
         }
+
+        bdrv_set_buffer_alignment(bs, 4096);
     }