Re: [md PATCH 09/18] md/raid10: stop print_conf from being too verbose.

NeilBrown
2016-06-10T06:47:44Z

[-- Attachment #1: Type: text/plain, Size: 2873 bytes --]

On Sat, Jun 04 2016, Shaohua Li wrote:

> On Fri, Jun 03, 2016 at 08:48:33AM +1000, Neil Brown wrote:
>> On Fri, Jun 03 2016, John Stoffel wrote:
>>
>> > NeilBrown> raid10 arrays can usefully be very large. When they are,
>> > NeilBrown> the noise generated by print_conf can over-whelm logs. So
>> > NeilBrown> truncate the listing at 16 devices.
>> >
>> > Why is this too noisy and how often does this print_conf() happen?
>> > And why 16 devices? I guess I don't like the magic number of 16 here,
>> > I'd prefer it to be a define, and possibly even something that can by
>> > dynamically changed.
>>
>> print_conf happens whenever a device becomes active in the array, or a
>> device is removed from the array (usually because it has failed).
>>
>> I got 16 by choosing a random number and multiplying by 4 (or maybe by
>> raising 2 to that power) :-)
>>
>> More seriously, I guessed that most arrays were 16 devices or less, so
>> this would not affect most arrays.
>>
>> I definitely don't think it needs a define. I'm very tempted to remove
>> print_conf() completely, but it is sometimes useful. So having it
>> present as long as it isn't a burden seems reasonable.
>>
>> If configuration was important I would change it to use pr_debug(), but
>> then the default would be to not see the messages at all, and they can
>> be useful in diagnosing problems reported on mailing lists.
>>
>> >
>> > But how big a problem is this really? And what about for big RAID5/6
>> > arrays as well?
>>
>> When you have 2000 devices in your RAID10 and half of them are removed
>> at once, it currently reports on 2,000,000 devices. With the patch it
>> is only 32000. Still possibly too many.
>>
>> If you have 2000 devices in your RAID5/6 and half of them are removed,
>> you have other problems.
>>
>> >
>> > Or would it be also good to condence the output of print_conf()
>> > itself?
>>
>> Probably a very good idea. Maybe the default could print a fixed-size
>> summary and the rest goes in pr_debug()??
>>
>> >
>> > Of if it's noise, why not just remove it completely? Can this
>> > information be found in /proc/mdstat instead?
>>
>> Its value is historical - trying to understand a past sequence of
>> events. For that, something in the logs beats something in /proc.
>>
>> >
>> > Sorry I havent' looked in the code deeply, but this just struck me as
>> > a change that might not be ideal.
>>
>> Fair enough - your comments are very valid. I'm not really sure what is
>> best. I only know what is worst :-) and want to avoid that.
>
> I would prefer pr_debug. pr_debug can be enabled dynamically. If the info is
> helpful for diagnosing, enabling it is simple.

Fair enough. Please drop this patch. I'll come up with an improved proposal
and resubmit.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]