Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Device-mapper Development (http://www.linux-archive.org/device-mapper-development/)
-   -   Test chunk size against both origin and snapshot sector size (http://www.linux-archive.org/device-mapper-development/341680-test-chunk-size-against-both-origin-snapshot-sector-size.html)

Mikulas Patocka 03-15-2010 05:04 AM

Test chunk size against both origin and snapshot sector size
 
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;
}

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Mike Snitzer 03-15-2010 01:52 PM

Test chunk size against both origin and snapshot sector size
 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Mikulas Patocka 03-15-2010 02:10 PM

Test chunk size against both origin and snapshot sector size
 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Mike Snitzer 03-15-2010 02:30 PM

Test chunk size against both origin and snapshot sector size
 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Mikulas Patocka 03-15-2010 02:52 PM

Test chunk size against both origin and snapshot sector size
 
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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

Mikulas Patocka 03-15-2010 02:54 PM

Test chunk size against both origin and snapshot sector size
 
> > 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

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


All times are GMT. The time now is 01:56 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.