From mboxrd@z Thu Jan 1 00:00:00 1970 From: Mike Snitzer Subject: Re: [PATCH 4/4] Test chunk size against both origin and snapshot sector size Date: Mon, 15 Mar 2010 11:30:31 -0400 Message-ID: <20100315153031.GA14350@redhat.com> References: <20100315145224.GE6853@redhat.com> Reply-To: device-mapper development Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Mikulas Patocka Cc: device-mapper development , Alasdair G Kergon List-Id: dm-devel.ids On Mon, Mar 15 2010 at 11:10am -0400, Mikulas Patocka wrote: > > > On Mon, 15 Mar 2010, Mike Snitzer wrote: > > > On Mon, Mar 15 2010 at 2:04am -0400, > > Mikulas Patocka 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 > > > > > > --- > > > 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