[dm-devel] [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 at redhat.com> wrote:
> 
> > 
> > 
> > On Mon, 15 Mar 2010, Mike Snitzer wrote:
> > 
> > > On Mon, Mar 15 2010 at  2:04am -0400,
> > > Mikulas Patocka <mpatocka at 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 at 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