From owner-svn-src-all@freebsd.org Wed Nov 7 19:46:19 2018 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 EA51B112D546 for ; Wed, 7 Nov 2018 19:46:18 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-it1-x130.google.com (mail-it1-x130.google.com [IPv6:2607:f8b0:4864:20::130]) (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 4A8A1764AC for ; Wed, 7 Nov 2018 19:46:18 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-it1-x130.google.com with SMTP id d6so24660658itl.4 for ; Wed, 07 Nov 2018 11:46:18 -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=0fEepdcobfoFrANBARpij6dGx1TimEQj48VbeAF/BfQ=; b=HvCD7//8f1t2SJnkhb+yQqAUcpccx12wYfrr9Y+cti1Tvl5H9KnDful6dM3I2H3Wyv AEDFxjddEVtdV2s5i5A891U8D9ClbRKkJ/DinTWG5AsAKJ7q/P/h+jQoGl5FU2C2F0pQ TMkVAGqBOGNpWoZGYqs+Tx/0xkcmW1yusJK8uSKcuNyzsN9Gp+3BsahlefRUtDnUorLP dPjJ7zvmn+ODOFfNUczjH7Bk7JWYvq8vZfl2myX75Yz27e5nxG4lsseW0yYsRJgqFkiE VqloeRBUDas0gUM5T7xCNMuvwfefNgdmCCf4UM8YuRXhOd2Z2zgjJ/o/tPHcX4uwxeVU zrIA== 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=0fEepdcobfoFrANBARpij6dGx1TimEQj48VbeAF/BfQ=; b=eWw9c+bTBAlUuXHsZsEdVZYYJ2gC66VtFtk1KODvqpEte3ml0jgQuVcN3S1u2b1fiY RqZYQeIjGUuRzxsI0jM9EK0VICsfRbI7i01vlpNuWRwK/uWiKC5R+4tRwLHafyQQEsLG siiLp4RPKsvuV3a062lAKy215HPmMgmpr4KcNx7TJyn+EbknlIPWjYIoHN01Xe+zZPVX 63KiKoOmULqxNHjEpJ5cvxFEyvGy9NkbEZdNr/tg4IuyxQxVLRibhz02zbEYPAqNNyBn s4KKuwEVclcnEA+nhQ3RDpxZm9hFaNJcyDJ9hOPpNnazIX3FOFKQSyX6iMyCB0aF9E7c tyLg== X-Gm-Message-State: AGRZ1gL4ZS8JkBwLSc0HM8/ftEzSLjg0vtmWW8bzuU2AnSeg156+a+/r fki9eVWxPCgoWTFYe7HDdMZhpeh3ROruWMmfJs4lqA== X-Google-Smtp-Source: AJdET5cjl2YxU12CyqLUQ/kfr+p1nP4LlCpVIxvavg+Rwej+nsrqJT8nPPuzRRpu7e8SyKsljDLCZ+JGSgM6ll1kiOw= X-Received: by 2002:a24:5f94:: with SMTP id r142-v6mr1424608itb.171.1541619977468; Wed, 07 Nov 2018 11:46:17 -0800 (PST) MIME-Version: 1.0 References: <1541606248.52306.42.camel@freebsd.org> <201811071606.wA7G6mQW001639@pdx.rh.CN85.dnsmgr.net> In-Reply-To: <201811071606.wA7G6mQW001639@pdx.rh.CN85.dnsmgr.net> From: Warner Losh Date: Wed, 7 Nov 2018 12:46:06 -0700 Message-ID: Subject: Re: svn commit: r340187 - head/sys/geom To: "Rodney W. Grimes" Cc: Ian Lepore , Maxim Sobolev , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org, Pawel Jakub Dawidek X-Rspamd-Queue-Id: 4A8A1764AC X-Spamd-Result: default: False [-3.29 / 200.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.996,0]; R_DKIM_ALLOW(-0.20)[bsdimp-com.20150623.gappssmtp.com]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; NEURAL_HAM_SHORT(-0.70)[-0.703,0]; NEURAL_HAM_LONG(-0.99)[-0.994,0]; MIME_GOOD(-0.10)[multipart/alternative,text/plain]; PREVIOUSLY_DELIVERED(0.00)[svn-src-all@freebsd.org]; DMARC_NA(0.00)[bsdimp.com]; TO_MATCH_ENVRCPT_SOME(0.00)[]; DKIM_TRACE(0.00)[bsdimp-com.20150623.gappssmtp.com:+]; MX_GOOD(-0.01)[cached: ALT1.aspmx.l.google.com]; RCPT_COUNT_SEVEN(0.00)[7]; RCVD_IN_DNSWL_NONE(0.00)[0.3.1.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; R_SPF_NA(0.00)[]; FORGED_SENDER(0.30)[imp@bsdimp.com,wlosh@bsdimp.com]; RCVD_TLS_LAST(0.00)[]; IP_SCORE(-0.58)[ipnet: 2607:f8b0::/32(-1.33), asn: 15169(-1.50), country: US(-0.09)]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; FROM_NEQ_ENVFROM(0.00)[imp@bsdimp.com,wlosh@bsdimp.com]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Server: mx1.freebsd.org 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: Wed, 07 Nov 2018 19:46:19 -0000 On Wed, Nov 7, 2018 at 9:07 AM Rodney W. Grimes < freebsd@pdx.rh.cn85.dnsmgr.net> wrote: > > On Tue, 2018-11-06 at 16:17 -0800, Rodney W. Grimes wrote: > > > > > > > > Rodney, this was actually my original intention, however then I > noticed in > > > > the GEOM code there is at least one case when BIO_FLUSH request is > being > > > > generated internally with bio_offset == mediasize and bio_lenth == > 0, so I > > > > thought there might be some need to allow such requests through. But > I'd > > > > happily go with the stricter rule if it does no harm. I simply don't > know > > > > enough about the intended use and the logic behind zero-length > transfers to > > > > make that call. > > > I am not sure enough on if mediasize is 0 based or not, > > > if it is then the error case should be fixed, and the > > > code you show below should also be fixed as it is > > > technically making a request beyond the end of device. > > > > > > I am also murky on why we are even doing a 0 size > > > operation and end of device, is that to validate > > > we can access all the media???If so then this wrong > > > code and wrong error return should be fixed as it > > > is off by 1. > > > > > > > > > > > > > > > -Max > > > > > > > > int > > > > g_io_flush(struct g_consumer *cp) > > > > { > > > > ... > > > > ????????bp = g_alloc_bio(); > > > > ????????bp->bio_cmd = BIO_FLUSH; > > > > ... > > > > ????????bp->bio_offset = cp->provider->mediasize; > > > The above should have a - 1 on it. > > > > > > > Unless offset > mediasize is specifically a signal to downstream code > > in some way about how the flush is to be performed. > > Could very well be, should be documented some place though. > > > Nearly identical code to create a BIO_FLUSH bio appears in ufs softdeps > > and in zfs. Before starting to arbitrarily change code that has worked > > since 2006, it might be a good idea to track down why these values are > > set the way they are. Unfortunately, there is no clue in the commit > > logs, but maybe the author (pjd@, cc'd) can englighten us. > > I agree with that take on the situation, and it is why I asked > for a revert and investigation, rather than trying to solve > why we suddenly fail some regression tests. > BIO_FLUSH is primarily done to force ordering points, since they are one of the only users of BIO_ORDERED in the system (the other is one place in UFS that shouldn't be doing BIO_ORDERED in theory, but in practice it's hard to fix. The exact values of the request don't really matter for the most part, though flushing past the end is seems ill defined. There's no way we'd force out blocks past the end. NVME, SCSI and ATA all implement BIO_FLUSH as either a NOP, or as simple command to flush all caches to stable media. Other drivers that I looked at appear to do something similar, though I've not looked at every single one of them in detail. Warner