Date: Wed, 7 Nov 2018 12:46:06 -0700 From: Warner Losh <imp@bsdimp.com> To: "Rodney W. Grimes" <rgrimes@freebsd.org> Cc: Ian Lepore <ian@freebsd.org>, Maxim Sobolev <sobomax@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org, Pawel Jakub Dawidek <pjd@freebsd.org> Subject: Re: svn commit: r340187 - head/sys/geom Message-ID: <CANCZdfrK-7vBETkhfQ9KkM4usrvjVh8wS_OZJiDmH5336DTsqQ@mail.gmail.com> In-Reply-To: <201811071606.wA7G6mQW001639@pdx.rh.CN85.dnsmgr.net> References: <1541606248.52306.42.camel@freebsd.org> <201811071606.wA7G6mQW001639@pdx.rh.CN85.dnsmgr.net>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrK-7vBETkhfQ9KkM4usrvjVh8wS_OZJiDmH5336DTsqQ>