From owner-freebsd-geom@freebsd.org Wed Nov 7 20:38:09 2018 Return-Path: Delivered-To: freebsd-geom@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 4A582112E603 for ; Wed, 7 Nov 2018 20:38:09 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: from mail-ot1-f46.google.com (mail-ot1-f46.google.com [209.85.210.46]) (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 A020878341 for ; Wed, 7 Nov 2018 20:38:08 +0000 (UTC) (envelope-from sobomax@sippysoft.com) Received: by mail-ot1-f46.google.com with SMTP id f24so16055341otl.5 for ; Wed, 07 Nov 2018 12:38:08 -0800 (PST) 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=STspOi0d1VmQ1Lj+mfH1BlgdWN91K5JlxwCFCyeO4IQ=; b=CNrZHBqtr5WpsAUnAybSFI/ie1pSPm+PziIbXxEqi9kXS/6YHkfNZC7dbKbxhazoVx 50EM5uvpS5+PMpwRGbKa/nmpGQw8UOCNyoKrCK0t/njUf78TGjY79wzwlVJfukfY+/ER /LHLiLU47fGRXdg7djnY9YuqTHJmCmMPriCYzYOd0u6OE+nrwrJwkVWTBwiUSDQUESQL CaWAX+AMHAv3Q66MBbnPaBQXvjF52cga/cDJQbaQzWiFVrTfaQZM2/MalrAr7/oDoop0 SbKPhrTfm8DGtTUMLpDiCKj2OsXmjvXEmbmr6SPE6i59Fn0j0PcPKzCswPIwAlMMh+tT Gxqw== X-Gm-Message-State: AGRZ1gKjUMaps8Jjy4vjjpWvMYSxzE98oCpzjzhx6O/MIkXbpZXRXMss K3dikEJXXsbJXjjFkiIh0VMeS05+2JVLd0LMLw5p3w== X-Google-Smtp-Source: AJdET5cVQFUf2JcvlfImXDQTIxu5JUCPKAgd0IO++bhL6YqD7kTBUhgFRHrB7DPvkVDtTvNFdgyZbDdgMusyTueDrFA= X-Received: by 2002:a9d:2817:: with SMTP id m23mr1148880otb.168.1541622611341; Wed, 07 Nov 2018 12:30:11 -0800 (PST) MIME-Version: 1.0 References: <1541606248.52306.42.camel@freebsd.org> <201811071606.wA7G6mQW001639@pdx.rh.CN85.dnsmgr.net> In-Reply-To: From: Maxim Sobolev Date: Wed, 7 Nov 2018 12:30:00 -0800 Message-ID: Subject: Re: svn commit: r340187 - head/sys/geom To: Warner Losh Cc: rgrimes@freebsd.org, Ian Lepore , Pawel Jakub Dawidek , Poul-Henning Kamp , freebsd-geom@freebsd.org Content-Type: multipart/mixed; boundary="000000000000222c6e057a18fc7b" X-Rspamd-Queue-Id: A020878341 X-Spamd-Result: default: False [-3.93 / 200.00]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.998,0]; FROM_HAS_DN(0.00)[]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17]; NEURAL_HAM_LONG(-1.00)[-0.996,0]; HAS_ATTACHMENT(0.00)[]; PREVIOUSLY_DELIVERED(0.00)[freebsd-geom@freebsd.org]; DMARC_NA(0.00)[freebsd.org]; MIME_GOOD(-0.10)[multipart/mixed,multipart/alternative,text/plain]; RCPT_COUNT_FIVE(0.00)[6]; TO_MATCH_ENVRCPT_SOME(0.00)[]; MX_GOOD(-0.01)[cached: alt1.aspmx.l.google.com]; NEURAL_HAM_SHORT(-0.93)[-0.929,0]; RCVD_IN_DNSWL_NONE(0.00)[46.210.85.209.list.dnswl.org : 127.0.5.0]; IP_SCORE(-1.00)[ipnet: 209.85.128.0/17(-3.39), asn: 15169(-1.50), country: US(-0.09)]; FORGED_SENDER(0.30)[sobomax@freebsd.org,sobomax@sippysoft.com]; R_DKIM_NA(0.00)[]; RCVD_TLS_LAST(0.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; FROM_NEQ_ENVFROM(0.00)[sobomax@freebsd.org,sobomax@sippysoft.com]; RCVD_COUNT_TWO(0.00)[2] X-Rspamd-Server: mx1.freebsd.org X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 07 Nov 2018 20:38:09 -0000 --000000000000222c6e057a18fc7b Content-Type: text/plain; charset="UTF-8" Here are two other possible approaches to fix the issue in question without disrupting special "sector after the last one" handling logic. One patches DIOCGDELETE code to not accept requests clearly beyond the provider's media boundary and the other one extends g_delete_data() to convert bio_length == 0 after biowait() into an error. Both effectively achieve the same result in my tests, but the g_delete_data() version has a chance to disrupt other kernel users. I am leaning towards DIOCGDELETE patch being the least evil. I would open phabricator request once I collect some input. Thanks! -Max On Wed, Nov 7, 2018 at 11:46 AM Warner Losh wrote: > > > 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 > --000000000000222c6e057a18fc7b Content-Type: application/octet-stream; name="patch2.diff" Content-Disposition: attachment; filename="patch2.diff" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_jo7m89301 ZGlmZiAtLWdpdCBhL3N5cy9nZW9tL2dlb21faW8uYyBiL3N5cy9nZW9tL2dlb21faW8uYwppbmRl eCA3Mzg5NWIzMGY3Li42NGUwOTVmZTQzIDEwMDY0NAotLS0gYS9zeXMvZ2VvbS9nZW9tX2lvLmMK KysrIGIvc3lzL2dlb20vZ2VvbV9pby5jCkBAIC0xMDA0LDYgKzEwMDQsMTYgQEAgZ19kZWxldGVf ZGF0YShzdHJ1Y3QgZ19jb25zdW1lciAqY3AsIG9mZl90IG9mZnNldCwgb2ZmX3QgbGVuZ3RoKQog CWJwLT5iaW9fZGF0YSA9IE5VTEw7CiAJZ19pb19yZXF1ZXN0KGJwLCBjcCk7CiAJZXJyb3IgPSBi aW93YWl0KGJwLCAiZ2RlbGV0ZSIpOworCWlmICgoZXJyb3IgPT0gMCkgJiYgKGJwLT5iaW9fbGVu Z3RoID09IDApKSB7CisJCS8qCisJCSAqIGdfaW9fcmVxdWVzdCgpIGhhcyBhIHNwZWNpYWwgaGFu ZGxpbmcgd2hlbiBwcm9jZXNzaW5nCisJCSAqIHJlcXVlc3RzIHRvIHRoZSBzZWN0b3IgaW1tZWRp YXRlbHkgYWZ0ZXIgcHJvdmlkZXJzCisJCSAqIG1lZGlhIGJvdW5kYXJ5LiBTdWNjZXNzIGlzIHJl dHVybmVkIGZvciBzdWNoIHJlcXVlc3RzLAorCQkgKiB3aXRoIGJpb19sZW5ndGggc2V0IHRvIDAu IFRyYW5zbGF0ZSB0aGlzIGludG8gcHJvcGVyCisJCSAqIEVJTy4KKwkJICovCisJCWVycm9yID0g RUlPOworCX0KIAlnX2Rlc3Ryb3lfYmlvKGJwKTsKIAlyZXR1cm4gKGVycm9yKTsKIH0K --000000000000222c6e057a18fc7b Content-Type: application/octet-stream; name="patch1.diff" Content-Disposition: attachment; filename="patch1.diff" Content-Transfer-Encoding: base64 Content-ID: X-Attachment-Id: f_jo7m88zo0 ZGlmZiAtLWdpdCBhL3N5cy9nZW9tL2dlb21fZGV2LmMgYi9zeXMvZ2VvbS9nZW9tX2Rldi5jCmlu ZGV4IDk3ODQxNmI1YTYuLjdlMWQ4NDA2ZmUgMTAwNjQ0Ci0tLSBhL3N5cy9nZW9tL2dlb21fZGV2 LmMKKysrIGIvc3lzL2dlb20vZ2VvbV9kZXYuYwpAQCAtNTEyLDYgKzUxMiwyMCBAQCBnX2Rldl9p b2N0bChzdHJ1Y3QgY2RldiAqZGV2LCB1X2xvbmcgY21kLCBjYWRkcl90IGRhdGEsIGludCBmZmxh Zywgc3RydWN0IHRocmVhZAogCQkJZXJyb3IgPSBFSU5WQUw7CiAJCQlicmVhazsKIAkJfQorCQlp ZiAoKGNwLT5wcm92aWRlci0+bWVkaWFzaXplID4gMCkgJiYKKwkJICAgIChvZmZzZXQgPj0gY3At PnByb3ZpZGVyLT5tZWRpYXNpemUpKSB7CisJCQkvKgorCQkJICogQ2F0Y2ggb3V0LW9mLWJvdW5k cyByZXF1ZXN0cyBoZXJlLiBUaGUgcHJvYmxlbSBpcworCQkJICogdGhhdCBkdWUgdG8gaGlzdG9y aWNhbCBHRU9NIEkvTyBpbXBsZW1lbnRhdGlvbgorCQkJICogcGVjdWxhdGl0aWVzLCBnX2RlbGV0 ZV9kYXRhKCkgd291bGQgYWx3YXlzIHJldHVybgorCQkJICogc3VjY2VzcyBmb3IgcmVxdWVzdHMg c3RhcnRpbmcganVzdCB0aGUgbmV4dCBieXRlCisJCQkgKiBhZnRlciBwcm92aWRlcnMgbWVkaWEg Ym91bmRhcnkuIENvbmRpdGlvbiBjaGVjayBvbgorCQkJICogbm9uLXplcm8gbWVkaWEgc2l6ZSwg c2luY2UgdGhhdCBjb25kaXRpb24gd291bGQKKwkJCSAqIChtb3N0IGxpa2VseSkgY2F1c2UgRU5Y SU8gaW5zdGVhZC4KKwkJCSAqLworCQkJZXJyb3IgPSBFSU87CisJCQlicmVhazsKKwkJfQogCQl3 aGlsZSAobGVuZ3RoID4gMCkgewogCQkJY2h1bmsgPSBsZW5ndGg7CiAJCQlpZiAoZ19kZXZfZGVs X21heF9zZWN0b3JzICE9IDAgJiYgY2h1bmsgPgo= --000000000000222c6e057a18fc7b--