On Tuesday May 19, raziebe@xxxxxxxxx wrote:
> md to support page size chunks in the case of raid 0
> Signed-off-by: raziebe@xxxxxxxxx
> md.c | 28 +++++++++++++++++++++-------
> 1 file changed, 21 insertions(+), 7 deletions(-)
> ---
> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 279007a..aab183e 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c
> @@ -443,9 +443,13 @@ static inline sector_t calc_dev_sboffset(struct block_device *bdev)
> static sector_t calc_num_sectors(mdk_rdev_t *rdev, unsigned chunk_size)
> {
> sector_t num_sectors = rdev->sb_start;
> + if (chunk_size) {
> + sector_t chunk_sects = chunk_size>>9;
> + sector_t x = num_sectors;
> + sector_div(x, chunk_sects);
> + num_sectors = x*chunk_sects;
> + }
>
> - if (chunk_size)
> - num_sectors &= ~((sector_t)chunk_size/512 - 1);
> return num_sectors;
> }
That's OK.... though you have removed the blank line separating the
variable declarations from the code. I like to keep that blank line
there.
And you have added a blank line before the "return", which I only
mention because.....
>
> @@ -3518,11 +3522,11 @@ min_sync_store(mddev_t *mddev, const char *buf, size_t len)
>
> /* Must be a multiple of chunk_size */
> if (mddev->chunk_size) {
> - if (min & (sector_t)((mddev->chunk_size>>9)-1))
> + unsigned long long temp = min;
> + if (sector_div(temp, (mddev->chunk_size>>9)))
> return -EINVAL;
> }
> mddev->resync_min = min;
> -
> return len;
> }
You have removed the blank line before the return here ??? consistency
is a good thing.
And 'temp' should be 'sector_t'. 'sector_div' requires a 'sector_t'
for the first argument.
>
> @@ -3555,7 +3559,8 @@ max_sync_store(mddev_t *mddev, const char *buf, size_t len)
>
> /* Must be a multiple of chunk_size */
> if (mddev->chunk_size) {
> - if (max & (sector_t)((mddev->chunk_size>>9)-1))
> + unsigned long long temp = max;
> + if (sector_div(temp, (mddev->chunk_size>>9)))
> return -EINVAL;
Again, temp must be sector_t.
> }
> mddev->resync_max = max;
> @@ -3996,14 +4001,23 @@ static int do_md_run(mddev_t * mddev)
> chunk_size, MAX_CHUNK_SIZE);
> return -EINVAL;
> }
> +
> /*
> * chunk-size has to be a power of 2
> */
> - if ( (1 << ffz(~chunk_size)) != chunk_size) {
> + if ((1 << ffz(~chunk_size)) != chunk_size &&
> + mddev->level != 0) {
> printk(KERN_ERR "chunk_size of %d not valid\n", chunk_size);
> return -EINVAL;
> }
I wold really like to remove any knowledge about specific raid levels
from the common (md.c) code and keep it all in the personality modules
(is that a job for you Andre ??).
So I definitely don't want to add a test for ->level here.
So I would like to see the tests for chunk_size removed do_md_run and
included in each personalities ->run function. This would be a series
of patches that adds the checks in ->run where needed, then removes it
from md.c. Would you like to do that?
> -
> + /*
> + * raid0 chunk size has to divide by a page
> + */
> + if (mddev->level == 0 && (chunk_size % PAGE_SIZE)) {
> + printk(KERN_ERR "chunk_size of %d not valid\n",
> + chunk_size);
> + return -EINVAL;
> + }
Why should the chunk_size be a multiple of PAGE_SIZE?
I suspect it should be a multiple of hardsect_size for each component
device (which, thanks to blk_queue_stack_limits, we can check by just
checking the hardsect_size of the mddev device after all the calls to
blk_queue_stack_limits in create_strip_zones, or in raid0_run.
And again, these checks need to move to raid0.c
Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
