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:
signature.asc
Description: PGP signature
