From owner-svn-src-all@freebsd.org Tue Feb 19 01:20:18 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 49E4914F45C1; Tue, 19 Feb 2019 01:20:18 +0000 (UTC) (envelope-from pkelsey@gmail.com) Received: from mail-it1-x12e.google.com (mail-it1-x12e.google.com [IPv6:2607:f8b0:4864:20::12e]) (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 CD49F8FE20; Tue, 19 Feb 2019 01:20:17 +0000 (UTC) (envelope-from pkelsey@gmail.com) Received: by mail-it1-x12e.google.com with SMTP id f10so2394435ita.4; Mon, 18 Feb 2019 17:20:17 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ieee.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=XjbzkbhYsl+OFmxjs9XuQPFQxFOG1Z62oDXRpIuj3Wc=; b=X/Gm9Ijg5OCfp9ZfI+9NsQ5z1zjDccYROo8vMfe1p1WDSKjXCzc6JM7IjhDXp036sB Iu9f3qKRasdmhGm/T6MLL56HmfE/5s70vJSQFGNGpVISSsYqoGXolA2g6MkmWk6jLUW8 4SwaZVmMQXLlRM0OIHvEV9lj94Hxb8WbEaqTE= 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=XjbzkbhYsl+OFmxjs9XuQPFQxFOG1Z62oDXRpIuj3Wc=; b=ns2c6J9nhVC7Licq+/tbpRJxRwtQ9GQhgDIOfy2srxT8c2mFbcRJLdoRzmDsmm5QE8 xeDrrL9cglX/0Na5MzPO0UP7v9n3vMQkh5qVrdm3AiZe/N14JOGWTc4p0ajel7VnqYnW l3hJ+WPir2hM1fEnCvMpQ2Iax8XPb7bIsBConP+nR74dd+AwV6Gk09rPaRJZapUgCRwy VfzaDZZMXbVhZstJviJs3PodWDhugA9MekQvS3bNdgRUoAm12gSQIAntt8gYhvbt1IRu u4sVw53mnXP/N0T7bYFD+GKv2ghIW/9BTtCTIrWmJJliTynx4FMlPn59ZQ6VU1uWYw8I JqqQ== X-Gm-Message-State: AHQUAuY8kvlSESZnV4o5/j+WjGsDjyB6jELAms583kKeTMFKzMo2Otjf 0kb8DmBTMOccuBZiNWTLYkCGWQaMAPyKOrX1ogte5A== X-Google-Smtp-Source: AHgI3IZ8Nknyf/bS/wOlKNNfwCMM+iEvDAwFi/Mk8nSMEW9r2KJfXzJhLSGc3/rDzaAABhHz3FqdjwzLcJ9K71n7uKY= X-Received: by 2002:a6b:f70a:: with SMTP id k10mr17279233iog.68.1550539217106; Mon, 18 Feb 2019 17:20:17 -0800 (PST) MIME-Version: 1.0 References: <201902181751.x1IHp9gc006395@pdx.rh.CN85.dnsmgr.net> In-Reply-To: From: Patrick Kelsey Date: Mon, 18 Feb 2019 20:20:04 -0500 Message-ID: Subject: Re: svn commit: r344238 - head/stand/common To: Warner Losh Cc: "Rodney W. Grimes" , Ian Lepore , Toomas Soome , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org X-Rspamd-Queue-Id: CD49F8FE20 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.72 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.72)[-0.716,0]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; REPLY(-4.00)[] 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: Tue, 19 Feb 2019 01:20:18 -0000 On Mon, Feb 18, 2019 at 3:15 PM Warner Losh wrote: > > > 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). > > To clarify, I recently committed some boot code patches that were the result of me investigating a problem on a machine that I have that initially presented as 'upgraded a ZFS machine to 11.2 and it appeared to hang during boot'. I spent a lot of time root causing the issue I was having on the chance that it might be a/the root cause of problems others have encountered that have a similar summary description. However, it turned out the root cause of my problem was a firmware bug in a 3Ware 9650SE related to multi-sector I/Os being issued to either 'suitably large' LBAs or perhaps LBAs 'suitably close' to the end of the disk causing 60 second I/O timeouts each, and moving to 11.2 triggered it because in 11.2, the ZFS code was modified to look at all the ZFS labels, two of which are very close to the end of the disk. Thankfully my problem was fixed by installing the latest available firmware version for that card (otherwise, the hack needed in the loader would have been to break up multi-sector I/Os 'near' the end of the disk and issue them in descending LBA order). The patches I committed were for issues I found during my investigation that were unrelated to my problem, one of which was the potential for a buffer overrun in the ZFS support code (which I think could only occur on systems running an ashift=9 ZFS configuration on 4K drives), and the other of which was the resolution of an inconsistency between the ZFS probing behavior of zfsboot and loader. -Patrick