[PATCH 4/4] Test chunk size against both origin and snapshot sector size

[PATCH 4/4] Test chunk size against both origin and snapshot sector size

[Mikulas Patocka]

Test chunk size against both origin and snapshot sector size

Don't allow chunk size smaller than either origin or snapshot logical
sector size. Reading or writing data unaligned to sector size is not allowed
and causes immediate errors.

Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>

---
 drivers/md/dm-exception-store.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c
===================================================================
--- linux-2.6.34-rc1-devel.orig/drivers/md/dm-exception-store.c	2010-03-12 14:38:31.000000000 +0100
+++ linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c	2010-03-12 14:39:56.000000000 +0100
@@ -173,7 +173,9 @@ int dm_exception_store_set_chunk_size(st
 
 	/* Validate the chunk size against the device block size */
 	if (chunk_size %
-	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9)) {
+	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9) ||
+	    chunk_size %
+	    (bdev_logical_block_size(dm_snap_origin(store->snap)->bdev) >> 9)) {
 		*error = "Chunk size is not a multiple of device blocksize";
 		return -EINVAL;
 	}

Re: [PATCH 4/4] Test chunk size against both origin and snapshot sector size

[Mike Snitzer]

On Mon, Mar 15 2010 at  2:04am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> Test chunk size against both origin and snapshot sector size
> 
> Don't allow chunk size smaller than either origin or snapshot logical
> sector size. Reading or writing data unaligned to sector size is not allowed
> and causes immediate errors.
> 
> Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> 
> ---
>  drivers/md/dm-exception-store.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c
> ===================================================================
> --- linux-2.6.34-rc1-devel.orig/drivers/md/dm-exception-store.c	2010-03-12 14:38:31.000000000 +0100
> +++ linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c	2010-03-12 14:39:56.000000000 +0100
> @@ -173,7 +173,9 @@ int dm_exception_store_set_chunk_size(st
>  
>  	/* Validate the chunk size against the device block size */
>  	if (chunk_size %
> -	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9)) {
> +	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9) ||
> +	    chunk_size %
> +	    (bdev_logical_block_size(dm_snap_origin(store->snap)->bdev) >> 9)) {
>  		*error = "Chunk size is not a multiple of device blocksize";
>  		return -EINVAL;
>  	}

Shouldn't we split these checks out so that we can have more precise
error reporting?  Ideally we'd share that chunk_size was not a multiple
of the "origin" or "snapshot" device's blocksize.

I was also thinking that we should avoid using %, e.g.: 
(chunk_size & (bdev_logical_block_size(...) - 1))

but AFAIK bdev_logical_block_size() may not be a power of 2 (MD allows
for obscure non-power of 2 blocksizes doesn't it?  Or is that just for
MD chunk and stripe size?).

Mike

Re: [PATCH 4/4] Test chunk size against both origin and snapshot sector size

[Mikulas Patocka]

On Mon, 15 Mar 2010, Mike Snitzer wrote:

> On Mon, Mar 15 2010 at  2:04am -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > Test chunk size against both origin and snapshot sector size
> > 
> > Don't allow chunk size smaller than either origin or snapshot logical
> > sector size. Reading or writing data unaligned to sector size is not allowed
> > and causes immediate errors.
> > 
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > 
> > ---
> >  drivers/md/dm-exception-store.c |    4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > Index: linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c
> > ===================================================================
> > --- linux-2.6.34-rc1-devel.orig/drivers/md/dm-exception-store.c	2010-03-12 14:38:31.000000000 +0100
> > +++ linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c	2010-03-12 14:39:56.000000000 +0100
> > @@ -173,7 +173,9 @@ int dm_exception_store_set_chunk_size(st
> >  
> >  	/* Validate the chunk size against the device block size */
> >  	if (chunk_size %
> > -	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9)) {
> > +	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9) ||
> > +	    chunk_size %
> > +	    (bdev_logical_block_size(dm_snap_origin(store->snap)->bdev) >> 9)) {
> >  		*error = "Chunk size is not a multiple of device blocksize";
> >  		return -EINVAL;
> >  	}
> 
> Shouldn't we split these checks out so that we can have more precise
> error reporting?  Ideally we'd share that chunk_size was not a multiple
> of the "origin" or "snapshot" device's blocksize.

You can split it to three messages ("not multiple of origin ... snapshot 
... both devices' blocksize"), but I think it's not so important to be 
worth code size increase.

> I was also thinking that we should avoid using %, e.g.: 
> (chunk_size & (bdev_logical_block_size(...) - 1))
> 
> but AFAIK bdev_logical_block_size() may not be a power of 2 (MD allows
> for obscure non-power of 2 blocksizes doesn't it?  Or is that just for
> MD chunk and stripe size?).
> 
> Mike

The Linux bio stack and page cache require that bdev_logical_block_size() 
is power of two. But the disks can be reformatted to other block sizes. 
I'm wondering, what happens then ... I suppose it wouldn't even allow to 
use the disk. I will try.

Mikulas

Re: [PATCH 4/4] Test chunk size against both origin and snapshot sector size

[Mike Snitzer]

On Mon, Mar 15 2010 at 11:10am -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Mon, 15 Mar 2010, Mike Snitzer wrote:
> 
> > On Mon, Mar 15 2010 at  2:04am -0400,
> > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > 
> > > Test chunk size against both origin and snapshot sector size
> > > 
> > > Don't allow chunk size smaller than either origin or snapshot logical
> > > sector size. Reading or writing data unaligned to sector size is not allowed
> > > and causes immediate errors.
> > > 
> > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > 
> > > ---
> > >  drivers/md/dm-exception-store.c |    4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > Index: linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c
> > > ===================================================================
> > > --- linux-2.6.34-rc1-devel.orig/drivers/md/dm-exception-store.c	2010-03-12 14:38:31.000000000 +0100
> > > +++ linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c	2010-03-12 14:39:56.000000000 +0100
> > > @@ -173,7 +173,9 @@ int dm_exception_store_set_chunk_size(st
> > >  
> > >  	/* Validate the chunk size against the device block size */
> > >  	if (chunk_size %
> > > -	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9)) {
> > > +	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9) ||
> > > +	    chunk_size %
> > > +	    (bdev_logical_block_size(dm_snap_origin(store->snap)->bdev) >> 9)) {
> > >  		*error = "Chunk size is not a multiple of device blocksize";
> > >  		return -EINVAL;
> > >  	}
> > 
> > Shouldn't we split these checks out so that we can have more precise
> > error reporting?  Ideally we'd share that chunk_size was not a multiple
> > of the "origin" or "snapshot" device's blocksize.
> 
> You can split it to three messages ("not multiple of origin ... snapshot 
> ... both devices' blocksize"), but I think it's not so important to be 
> worth code size increase.
> 
> > I was also thinking that we should avoid using %, e.g.: 
> > (chunk_size & (bdev_logical_block_size(...) - 1))
> > 
> > but AFAIK bdev_logical_block_size() may not be a power of 2 (MD allows
> > for obscure non-power of 2 blocksizes doesn't it?  Or is that just for
> > MD chunk and stripe size?).
> > 
> > Mike
> 
> The Linux bio stack and page cache require that bdev_logical_block_size() 
> is power of two.

OK, I'll have a look, but it sounds like we could use:
(chunk_size & (bdev_logical_block_size(...) - 1))

> But the disks can be reformatted to other block sizes. 
> I'm wondering, what happens then ... I suppose it wouldn't even allow to 
> use the disk. I will try.

Do you mean something like 512b logical and 4K physical?  Such devices
must perform the appropriate r-m-w.  A 4K formatted device will report
4K for both logical and physical (unless the device and format tool
allows for physical != logical).

Mike

Re: [PATCH 4/4] Test chunk size against both origin and snapshot sector size

[Mikulas Patocka]

On Mon, 15 Mar 2010, Mike Snitzer wrote:

> On Mon, Mar 15 2010 at 11:10am -0400,
> Mikulas Patocka <mpatocka@redhat.com> wrote:
> 
> > 
> > 
> > On Mon, 15 Mar 2010, Mike Snitzer wrote:
> > 
> > > On Mon, Mar 15 2010 at  2:04am -0400,
> > > Mikulas Patocka <mpatocka@redhat.com> wrote:
> > > 
> > > > Test chunk size against both origin and snapshot sector size
> > > > 
> > > > Don't allow chunk size smaller than either origin or snapshot logical
> > > > sector size. Reading or writing data unaligned to sector size is not allowed
> > > > and causes immediate errors.
> > > > 
> > > > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > > > 
> > > > ---
> > > >  drivers/md/dm-exception-store.c |    4 +++-
> > > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > Index: linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c
> > > > ===================================================================
> > > > --- linux-2.6.34-rc1-devel.orig/drivers/md/dm-exception-store.c	2010-03-12 14:38:31.000000000 +0100
> > > > +++ linux-2.6.34-rc1-devel/drivers/md/dm-exception-store.c	2010-03-12 14:39:56.000000000 +0100
> > > > @@ -173,7 +173,9 @@ int dm_exception_store_set_chunk_size(st
> > > >  
> > > >  	/* Validate the chunk size against the device block size */
> > > >  	if (chunk_size %
> > > > -	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9)) {
> > > > +	    (bdev_logical_block_size(dm_snap_cow(store->snap)->bdev) >> 9) ||
> > > > +	    chunk_size %
> > > > +	    (bdev_logical_block_size(dm_snap_origin(store->snap)->bdev) >> 9)) {
> > > >  		*error = "Chunk size is not a multiple of device blocksize";
> > > >  		return -EINVAL;
> > > >  	}
> > > 
> > > Shouldn't we split these checks out so that we can have more precise
> > > error reporting?  Ideally we'd share that chunk_size was not a multiple
> > > of the "origin" or "snapshot" device's blocksize.
> > 
> > You can split it to three messages ("not multiple of origin ... snapshot 
> > ... both devices' blocksize"), but I think it's not so important to be 
> > worth code size increase.
> > 
> > > I was also thinking that we should avoid using %, e.g.: 
> > > (chunk_size & (bdev_logical_block_size(...) - 1))
> > > 
> > > but AFAIK bdev_logical_block_size() may not be a power of 2 (MD allows
> > > for obscure non-power of 2 blocksizes doesn't it?  Or is that just for
> > > MD chunk and stripe size?).
> > > 
> > > Mike
> > 
> > The Linux bio stack and page cache require that bdev_logical_block_size() 
> > is power of two.
> 
> OK, I'll have a look, but it sounds like we could use:
> (chunk_size & (bdev_logical_block_size(...) - 1))
> 
> > But the disks can be reformatted to other block sizes. 
> > I'm wondering, what happens then ... I suppose it wouldn't even allow to 
> > use the disk. I will try.
> 
> Do you mean something like 512b logical and 4K physical?  Such devices
> must perform the appropriate r-m-w.  A 4K formatted device will report
> 4K for both logical and physical (unless the device and format tool
> allows for physical != logical).
> 
> Mike

No, I meant what happens if you format it with 514, 516, etc physical 
blocksize. the Linux clearly doesn't support it, so what will it do with 
it? I'll try when I finish tests on 4K.

Mikulas

Re: [PATCH 4/4] Test chunk size against both origin and snapshot sector size

[Martin K. Petersen]

>>>>> "Mikulas" == Mikulas Patocka <mpatocka@redhat.com> writes:
>> Do you mean something like 512b logical and 4K physical?  Such
>> devices must perform the appropriate r-m-w.  A 4K formatted device
>> will report 4K for both logical and physical (unless the device and
>> format tool allows for physical != logical).
>> 
>> Mike

Mikulas> No, I meant what happens if you format it with 514, 516, etc
Mikulas> physical blocksize. the Linux clearly doesn't support it, so
Mikulas> what will it do with it? I'll try when I finish tests on 4K.

We only support powers of two (except when the drive is formatted with
DIF but even in that case the logical_block_size is a power of two).

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 4/4] Test chunk size against both origin and snapshot sector size

[Mikulas Patocka]

> > The Linux bio stack and page cache require that bdev_logical_block_size() 
> > is power of two.
> 
> OK, I'll have a look, but it sounds like we could use:
> (chunk_size & (bdev_logical_block_size(...) - 1))

We could, but it doesn't matter anyway because it's just initialization 
and it's not performance critical.

Mikulas