From owner-svn-src-all@freebsd.org Mon Feb 18 17:51:13 2019 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id ABFCC14E5DD3; Mon, 18 Feb 2019 17:51:13 +0000 (UTC) (envelope-from freebsd@pdx.rh.CN85.dnsmgr.net) Received: from pdx.rh.CN85.dnsmgr.net (br1.CN84in.dnsmgr.net [69.59.192.140]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 394F074F6A; Mon, 18 Feb 2019 17:51:12 +0000 (UTC) (envelope-from freebsd@pdx.rh.CN85.dnsmgr.net) Received: from pdx.rh.CN85.dnsmgr.net (localhost [127.0.0.1]) by pdx.rh.CN85.dnsmgr.net (8.13.3/8.13.3) with ESMTP id x1IHp9B8006396; Mon, 18 Feb 2019 09:51:09 -0800 (PST) (envelope-from freebsd@pdx.rh.CN85.dnsmgr.net) Received: (from freebsd@localhost) by pdx.rh.CN85.dnsmgr.net (8.13.3/8.13.3/Submit) id x1IHp9gc006395; Mon, 18 Feb 2019 09:51:09 -0800 (PST) (envelope-from freebsd) From: "Rodney W. Grimes" Message-Id: <201902181751.x1IHp9gc006395@pdx.rh.CN85.dnsmgr.net> Subject: Re: svn commit: r344238 - head/stand/common In-Reply-To: To: Warner Losh Date: Mon, 18 Feb 2019 09:51:09 -0800 (PST) CC: Ian Lepore , Toomas Soome , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Reply-To: rgrimes@freebsd.org X-Mailer: ELM [version 2.4ME+ PL121h (25)] MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset=US-ASCII X-Rspamd-Queue-Id: 394F074F6A X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.92 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.92)[-0.917,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Feb 2019 17:51:13 -0000 > On Mon, Feb 18, 2019 at 7:31 AM Ian Lepore wrote: > > > On Mon, 2019-02-18 at 15:09 +0200, Toomas Soome wrote: > > > > On 18 Feb 2019, at 01:32, Ian Lepore wrote: > > > > > > > > Author: ian > > > > Date: Sun Feb 17 23:32:09 2019 > > > > New Revision: 344238 > > > > URL: https://svnweb.freebsd.org/changeset/base/344238 > > > > > > > > Log: > > > > Restore loader(8)'s ability for lsdev to show partitions within a bsd > > slice. > > > > > > > > I'm pretty sure this used to work at one time, perhaps long ago. It > > has > > > > been failing recently because if you call disk_open() with > > dev->d_partition > > > > set to -1 when d_slice refers to a bsd slice, it assumes you want it > > to > > > > open the first partition within that slice. When you then pass that > > open > > > > dev instance to ptable_open(), it tries to read the start of the 'a' > > > > partition and decides there is no recognizable partition type there. > > > > > > > > This restores the old functionality by resetting d_offset to the start > > > > of the raw slice after disk_open() returns. For good measure, > > d_partition > > > > is also set back to -1, although that doesn't currently affect > > anything. > > > > > > > > I would have preferred to make disk_open() avoid such rude > > assumptions and > > > > if you ask for partition -1 you get the raw slice. But the commit > > history > > > > shows that someone already did that once (r239058), and had to revert > > it > > > > (r239232), so I didn't even try to go down that road. > > > > > > > > > What was the reason for the revert? I still do think the current > > > disk_open() approach is not good because it does break the promise to > > > get MBR partition (see common/disk.h). > > > > > > Of course I can see the appeal for something like ?boot disk0:? but > > > case like that should be solved by iterating partition table, and not > > > by making API to do wrong thing - if I did ask to for disk0s0: I > > > really would expect to get disk0s0: and not disk0s0a: > > > > > > But anyhow, it would be good to understand the actual reasoning > > > behind that decision. > > > > > > > I have no idea. As is so often the case, the commit message for the > > revert said what the commit did ("revert to historic behavior") without > > any hint of why the change was made. One has to assume that it broke > > some working cases and people complained. > > > > Part of the problem for the disk_open() "api" is that there is not even > > a comment block with some hints in it. I was thinking one potential > > solution might instead of using "if (partition < 0)" we could define a > > couple magical partition number values, PNUM_GETBEST = -1, > > PNUM_RAWSLICE = -2, that sort of thing. But it would require carefully > > combing through the existing code looking at all calls to disk_open() > > and all usage of disk_devdesc.d_partition. > > > > I think that we should fix the disk_open() api. And then fix everything > that uses it to comply with the new rules. The current hodge-podge that we > have causes a number of issues for different environments that are only > mostly worked around. I've done some work in this area to make things more > regular, but it's still a dog's breakfast of almost-stackable devices that > we overload too many things on, resulting in the mis-mash we have today. > When the whole world was just MBR + BSD label, we could get away with it. > But now that we have GPT as well as GELI support and ZFS pool devices on > top of disk devices, the arrangements aren't working as well as could be > desired. Yes please. Could this be some of the cause of issues with legacy mbr boots that use to work, that now stop working? I have observed this on zfs in mbr on a machine here. I did not have time to debug it, it was faster to nuke and pave the machine that was critical to operations, the error came as part of a failed freenas upgrade to the 11.2 based version. -- Rod Grimes rgrimes@freebsd.org