On Wed, 2009-05-20 at 20:17 +1000, Neil Brown wrote:

On Wednesday May 20, maan@systemlinux.org wrote:

quoted

On Tue, May 19, 2009 at 10:47:56AM +1000, Neil Brown wrote:

quoted

And 'temp' should be 'sector_t'.  'sector_div' requires a 'sector_t'
for the first argument.

...

quoted

Again, temp must be sector_t.

How about rolling our own md_sector_div() which at least checks for
such bugs via the

        (void)(((typeof((temp)) *)0) == ((sector_t *)0))

trick?

nice. did not know typeof is available in c. 

Interesting idea..
You would still need to eyeball that code to make sure md_sector_div
was used instead of sector_div, though I guess that is more obvious..
Maybe sector_div should have that test in it globally...
Though I've actually hit more problems with the second arg not being
"unsigned long" like it should be.

quoted

quoted

quoted

@@ -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?

Sure, I can give it a try. Though I'm not sure I fully understand
what would be the difference between the checks in the individual
->run functions. Currently, in do_md_run() we check that

        * chunk_size <= MAX_CHUNK_SIZE
        * chunk_size is a power of two
        * the rdev is at least one chunk large

None of these checks depend on the raid level, so the above change
that allows chunk sizes which are not a power of two for raid0 would
be the only difference. Are you anticipating that the requirements
of the various raid levels with respect to chunk size will further
diverge in the future?

The second paragraph is already done (in for-next).  md.c doesn't
check chunksize any more, that is only checked in personality modules
that care.

What I was suggesting for you is the previous paragraph.  Removing all
knowledge about specific raid levels from md.c.
So where ever md.c tests the value for mddev->level, ask the question:
how can this information be extracted from the module rather than
having this hard-coded test.

Probably the easiest would be moving


	if (mddev->recovery_cp != MaxSector &&
	    mddev->level >= 1)
		printk(KERN_ERR "md: %s: raid array is not clean"
		       " -- starting background reconstruction\n",
		       mdname(mddev));

into various ->run methods.
Probably the hardest would be all the messing with LEVEL_MULTIPATH.
Somewhere in between would be the tests for which levels support
bitmaps.

In general, we just want to delay the test until we are in personality
code and do it there.

why not add a new method ? pers->validate_parms ?  

Does that make sense?

Thanks,
NeilBrown
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html