On Tue, 9 Nov 2010 09:23:49 -0800 Dan Williams <dan.j.williams@xxxxxxxxx> wrote: > On Tue, Nov 9, 2010 at 7:43 AM, Czarnowska, Anna > <anna.czarnowska@xxxxxxxxx> wrote: > >> > >> Neil wondered if we can repurpose validate_geometry for this case? It > >> is already charged with checking if a disk is suitable to be added to > >> an > >> array. > > > > I had a look at validate_geometry and I don't think it makes sense to modify it for Monitor. > > Validate_geometry is made for Create. When creating container validate_geometry doesn't check much for imsm. When adding to a volume all disks must be already in the same container and have some common free region. We want the check before we move a spare. And the whole spare is free (is not used by any arrays in that container). I think it is better to have smaller functions and use them as building blocks, than make a big function even more complicated for every special case. > > Yes, and no :-). There is a line to be drawn between refactoring and > adding complexity, and you are right that all things being equal > validate_geometry() as it stands today is an awkward fit. However, > the architecture rework Neil is doing is coming precisely from the > realization that we are generating lots of these special case routines > that are doing not much more than representing the same data to > slightly different contexts. The conclusion to be drawn is that our > interface from generic mdadm to the metadata handler is perhaps too > fine grained. Hence the refactoring and why in this case it is > worthwhile to at least ask the question: can a routine tasked with > validating disks are proper candidates to be added to an array / > container be trivially re-purposed to handle the case of validating > that a spare is suitable for an existing container? I agree with your > conclusion, but wanted to share why I think Neil asked the question, > hopefully I am not putting words in his mouth. > > -- > Dan I had a proper look at the code and I think that when I said "validate_geometry", I should have said "avail_size". We can probably just use the one method for both 'avail_size' and 'min_acceptable_spare_size' though I'm not sure currently what it would look like. So I'm happy to take that patch with min_acceptable_spare_size, and I will quite possibly revise it later. 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
