From owner-svn-src-head@freebsd.org Wed Aug 17 14:36:07 2016 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 5AA21BBD156; Wed, 17 Aug 2016 14:36:07 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from c.mail.sonic.net (c.mail.sonic.net [64.142.111.80]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 3531A1774; Wed, 17 Aug 2016 14:36:06 +0000 (UTC) (envelope-from nwhitehorn@freebsd.org) Received: from zeppelin.tachypleus.net ([128.54.117.176]) (authenticated bits=0) by c.mail.sonic.net (8.15.1/8.15.1) with ESMTPSA id u7HEa0P5006569 (version=TLSv1.2 cipher=DHE-RSA-AES128-SHA bits=128 verify=NOT); Wed, 17 Aug 2016 07:36:01 -0700 Subject: Re: svn commit: r304142 - head/usr.sbin/bsdinstall/partedit To: =?UTF-8?Q?Dag-Erling_Sm=c3=b8rgrav?= References: <201608150930.u7F9UL1V069576@repo.freebsd.org> <861t1n6749.fsf@desk.des.no> Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org From: Nathan Whitehorn Message-ID: <581c856c-826b-529e-c9c6-a397fb679708@freebsd.org> Date: Wed, 17 Aug 2016 07:36:00 -0700 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:45.0) Gecko/20100101 Thunderbird/45.1.0 MIME-Version: 1.0 In-Reply-To: <861t1n6749.fsf@desk.des.no> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Sonic-CAuth: UmFuZG9tSVb4EAMaBaW94IvkrbqJOuc277FbO3w/nQosgRFWeFnhLSfkhOOIqmsosANZbmQDQIMpj4Ejr+Kxe49oFwwBGrlh0blyFwEbqHo= X-Sonic-ID: C;JPrl6odk5hGnlqDx2xNB0g== M;Yrpa64dk5hGnlqDx2xNB0g== X-Spam-Flag: No X-Sonic-Spam-Details: 0.0/5.0 by cerberusd X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 17 Aug 2016 14:36:07 -0000 On 08/17/16 03:07, Dag-Erling Smørgrav wrote: > Nathan Whitehorn writes: >> As a note for people who weren't paying attention to the bug, we need >> to fix this in a better way outside of the constraints of getting 11.0 >> out the door. The system (gpart, the installer, ZFS, etc.) uses the >> reported GEOM stripesize for partition alignment and IO block size >> selection. If that is wrong, we should identify devices on which it is >> wrong and fix them, and maybe also add some global tunable that sets a >> floor on the numbers reported by GEOM_DISK. Hacking the installer like >> this is triage, which is fine, but not viable as a permanent solution >> to anything. > Modifying GEOM to report a bogus number when none is provided by the > lower layer(s) is absolutely not going to happen. You have absolutely > no idea what your proposed change will break. And you keep refusing to > address the fact that most drivers don't report a stripe size, except by > repeating your claim that they do, with no evidence to back it up. Feel > free to 'grep -r stripesize /usr/src/sys/dev'. Go on, I'll wait. And yet, if you look at the GEOM XML, it is reported and there. And, look, it's even right for the AF 512e disks in my machine! 512 4096 I've literally never seen a case where we don't already do the right thing here. The GEOM stripesize is, whether you like it or not, the way we have long ago decided to communicate the information about optimal alignment from the disk drivers to userland. We do a good job of it, too. It's correct, as far as I can tell, 100% of the time on all possible variants of AF disks. One could argue that calling this the "stripesize" is a hack, and I would agree, but it's what the operating system does and has done for many years. As for grepping, the CAM disk drivers are all in sys/cam, not sys/dev, as I'm sure you know, and you will find all the code that handles this there. > Your contention that the installer does not make policy decisions is > equally spurious. The installer makes many policy decisions, including > the disk layout, the size of the swap partition, the name of the pool, > the use of boot environments (which I dislike but am not allowed to > override), the number of filesets and their mountpoints (which I also > dislike and am not allowed to override either), etc. The Unix > philosophy is to push such decisions up the stack, not down. The > decision to align partitions on 4096-byte boundaries because we're not > sure of the correct number but know for a fact that using a smaller > number can have a huge impact on performance is the installer's to make. Those are all things that the operating system does not have defaults for: there are no tools like, say, gpart or newfs that layout disks in any even vaguely automated way, and so no tools that would ever have defaults for, say, the size of a swap partition except for the installer. As such, the defaults are quite properly in the installer. This is quite different: there are many tools that care about disk alignment (say, gpart) and, by default, use the GEOM stripesize. The installer is, after this patch, overriding what was meant to be a system-wide default. My concern is that pushing this into the installer means that newfs, zfs, gpart, etc., which all look at the GEOM stripesize for preferred alignment, will still have suboptimal behavior on systems affected by your patch. If we identified which drivers are reporting the wrong alignment, we could fix the whole system at a go by changing it there. As it is, we now have inconsistent default behavior for partitions between tools (the installer and sade will now use a different alignment than gpart on whatever systems you were trying to fix here) and between pre- and post-installation environments. You are papering over a bug in some unspecified driver with some unspecified disks by hacking the installer. This is fine as an expedient for 11.0, but is a ridiculous solution to the problem otherwise. We should just fix the driver for whatever weird disk you have in your machine (what is it, by the way?). Since making the reported "stripe size" match the physical sector size is what GEOM has done since at least 2009, I doubt very much that fixing any bugs in that reported value would have the weird unintended consequences you imply it might. -Nathan > > DES