From owner-svn-src-all@freebsd.org Mon Feb 18 20:14:43 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 6FF5614EA507 for ; Mon, 18 Feb 2019 20:14:43 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-qt1-x830.google.com (mail-qt1-x830.google.com [IPv6:2607:f8b0:4864:20::830]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id F141C83F3A for ; Mon, 18 Feb 2019 20:14:42 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-qt1-x830.google.com with SMTP id w4so20683843qtc.1 for ; Mon, 18 Feb 2019 12:14:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=xMdBmKAUyirc1/d4i/63DqIr+yBGhcb54pBAv+uoIa4=; b=VDjoWU/vUDuLnGicWRXIc0teYsEbuaIwxsYqGal1REaPqg+0yHH0b7+GTW81YfNiLu Wd5Q/55xBzyRAq9r80escB47dcuHKBgrF9Sc8aEvDCXqQFWOWGzRmhJR2S+cx2reOtJE R7IhIUsh897BNEApDN+0f4vtdRXz1vuhNPkgitsd98ZSMmViGkkV2io3yXqgiK2UucKW NvJonqMQ6F9KZgmwXvoz2KYGAz2WYi8+9apO6UcgVUn9dd0S691YpMwdXQO3XrttpLs4 hoKUjJ2hgfErYHoXygUaJiZNqmx+1EPAEgGvxrX64Nlkyq08m3RBoe5+AJJWX5dcChf8 kuvA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=xMdBmKAUyirc1/d4i/63DqIr+yBGhcb54pBAv+uoIa4=; b=auhJUpL8QHwn6/YQwvWf31oh63bc2xE6FnUmf6kOy3ziGSVP20KUbCD4mDp9wiNz7N P/PCjUkIPUG1Bo/B58tXZimu1K4Hem/jgUSuecE3BZc5ven0n9458vcynxcSdwIFNaLA UGeWt3FJasSNMYb7rpEu7No8FUMm2KsFkeTZaz5JeGNyCI1ddp2c8xHu4QQU2/Rf9h5Z bCR2hJnQAqKaxCvMCbMBT4dESzL1kVtzi71oIANfJhf0H3J5F2qm+TfaclfO6l9BDXX+ 7ZP7VTMMC1F3bmAgtDfVVjbUdDB9+/Z+JiPSuEio5y4g+cP2obbE8FBDY2hFl7inLDaP CsJg== X-Gm-Message-State: AHQUAubpU0xKKk5TedLAZF4gNZ/KFuD3ek/sBpHc9gWiSdgU7gl3rqjh 7mFMu6GJZDFZTEaBhEkSLLt26oltPRsH01FxNL6W4Q== X-Google-Smtp-Source: AHgI3IZQD+7QpjQGnPqCeJn0dQ8bXvq07fn4h3FQcrkFw0CiHZaA8TF8HlQDCNtAf+eMU3DsMP3921ACP1fTinJk1r4= X-Received: by 2002:a0c:9c89:: with SMTP id i9mr18513307qvf.153.1550520882374; Mon, 18 Feb 2019 12:14:42 -0800 (PST) MIME-Version: 1.0 References: <201902181751.x1IHp9gc006395@pdx.rh.CN85.dnsmgr.net> In-Reply-To: <201902181751.x1IHp9gc006395@pdx.rh.CN85.dnsmgr.net> From: Warner Losh Date: Mon, 18 Feb 2019 13:14:31 -0700 Message-ID: Subject: Re: svn commit: r344238 - head/stand/common To: "Rodney W. Grimes" Cc: Ian Lepore , Toomas Soome , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: F141C83F3A X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.95 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[]; NEURAL_HAM_SHORT(-0.95)[-0.947,0] Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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 20:14:43 -0000 On Mon, Feb 18, 2019 at 10:51 AM Rodney W. Grimes < freebsd@pdx.rh.cn85.dnsmgr.net> wrote: > > 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. > I think the MBR issues are the same ones that Patrick Kelsey is looking into. He has a differential review. If the upgrade was to FreeBSD 11.2, however, that pre-dates most of the work we've done on the loader, so the root cause of that issue may be different. And it may already be fixed in stable/11 as we've since merged everything in current into stable/11 (except for a few things that changed the defaults in a user-visible way). Warner