Date: Wed, 7 Nov 2018 12:30:00 -0800 From: Maxim Sobolev <sobomax@freebsd.org> To: Warner Losh <imp@bsdimp.com> Cc: rgrimes@freebsd.org, Ian Lepore <ian@freebsd.org>, Pawel Jakub Dawidek <pjd@freebsd.org>, Poul-Henning Kamp <phk@phk.freebsd.dk>, freebsd-geom@freebsd.org Subject: Re: svn commit: r340187 - head/sys/geom Message-ID: <CAH7qZfsmh2epo_0AVanmmKu6J=F6vBh2GdAXTtRGCwrURCRkZQ@mail.gmail.com> In-Reply-To: <CANCZdfrK-7vBETkhfQ9KkM4usrvjVh8wS_OZJiDmH5336DTsqQ@mail.gmail.com> References: <1541606248.52306.42.camel@freebsd.org> <201811071606.wA7G6mQW001639@pdx.rh.CN85.dnsmgr.net> <CANCZdfrK-7vBETkhfQ9KkM4usrvjVh8wS_OZJiDmH5336DTsqQ@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--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 <imp@bsdimp.com> 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: <f_jo7m89301>
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: <f_jo7m88zo0>
X-Attachment-Id: f_jo7m88zo0
ZGlmZiAtLWdpdCBhL3N5cy9nZW9tL2dlb21fZGV2LmMgYi9zeXMvZ2VvbS9nZW9tX2Rldi5jCmlu
ZGV4IDk3ODQxNmI1YTYuLjdlMWQ4NDA2ZmUgMTAwNjQ0Ci0tLSBhL3N5cy9nZW9tL2dlb21fZGV2
LmMKKysrIGIvc3lzL2dlb20vZ2VvbV9kZXYuYwpAQCAtNTEyLDYgKzUxMiwyMCBAQCBnX2Rldl9p
b2N0bChzdHJ1Y3QgY2RldiAqZGV2LCB1X2xvbmcgY21kLCBjYWRkcl90IGRhdGEsIGludCBmZmxh
Zywgc3RydWN0IHRocmVhZAogCQkJZXJyb3IgPSBFSU5WQUw7CiAJCQlicmVhazsKIAkJfQorCQlp
ZiAoKGNwLT5wcm92aWRlci0+bWVkaWFzaXplID4gMCkgJiYKKwkJICAgIChvZmZzZXQgPj0gY3At
PnByb3ZpZGVyLT5tZWRpYXNpemUpKSB7CisJCQkvKgorCQkJICogQ2F0Y2ggb3V0LW9mLWJvdW5k
cyByZXF1ZXN0cyBoZXJlLiBUaGUgcHJvYmxlbSBpcworCQkJICogdGhhdCBkdWUgdG8gaGlzdG9y
aWNhbCBHRU9NIEkvTyBpbXBsZW1lbnRhdGlvbgorCQkJICogcGVjdWxhdGl0aWVzLCBnX2RlbGV0
ZV9kYXRhKCkgd291bGQgYWx3YXlzIHJldHVybgorCQkJICogc3VjY2VzcyBmb3IgcmVxdWVzdHMg
c3RhcnRpbmcganVzdCB0aGUgbmV4dCBieXRlCisJCQkgKiBhZnRlciBwcm92aWRlcnMgbWVkaWEg
Ym91bmRhcnkuIENvbmRpdGlvbiBjaGVjayBvbgorCQkJICogbm9uLXplcm8gbWVkaWEgc2l6ZSwg
c2luY2UgdGhhdCBjb25kaXRpb24gd291bGQKKwkJCSAqIChtb3N0IGxpa2VseSkgY2F1c2UgRU5Y
SU8gaW5zdGVhZC4KKwkJCSAqLworCQkJZXJyb3IgPSBFSU87CisJCQlicmVhazsKKwkJfQogCQl3
aGlsZSAobGVuZ3RoID4gMCkgewogCQkJY2h1bmsgPSBsZW5ndGg7CiAJCQlpZiAoZ19kZXZfZGVs
X21heF9zZWN0b3JzICE9IDAgJiYgY2h1bmsgPgo=
--000000000000222c6e057a18fc7b--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAH7qZfsmh2epo_0AVanmmKu6J=F6vBh2GdAXTtRGCwrURCRkZQ>