Re: svn commit: r340187 - head/sys/geom
On Wed, Nov 07, 2018 at 08:59:24AM -0800, Rodney W. Grimes wrote: > freebsd-geom is also probably a pretty short list. IMHO it's one of our mailing lists that became obsolete once the initial work was done. The bugbusters still assign to these lists, however. ISTM that some of these assignee lists' recent archives contain only bug assignments and spam :-/ mcl ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
On Wed, Nov 7, 2018 at 1:20 PM Poul-Henning Kamp wrote: > > In message < > canczdfrk-7vbetkhfq9kkm4usrvjvh8ws_ozjidmh5336dt...@mail.gmail.com> > , Warner Losh writes: > > >BIO_FLUSH is primarily done to force ordering points, > > Originally BIO_FLUSH was defined the way it is, to make it possible > to flush an isolated specified range on providers which support that > so that fsync(2) could be implemented that way. > > I can't remember the exact semantics of the two "magic" flush > operations (off=0,len=0) and (off=end,len=0) but they were different > from each other in some important aspect (Pawel?) > I couldn't find any place where the different types of flush are differentiated (nothing in CAM for sure), though I suppose I could have missed something. All the CAM drivers just use it to push data to the device and don't look at the flags. Warner ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
In message , Warner Losh writes: >BIO_FLUSH is primarily done to force ordering points, Originally BIO_FLUSH was defined the way it is, to make it possible to flush an isolated specified range on providers which support that so that fsync(2) could be implemented that way. I can't remember the exact semantics of the two "magic" flush operations (off=0,len=0) and (off=end,len=0) but they were different from each other in some important aspect (Pawel?) -- Poul-Henning Kamp | UNIX since Zilog Zeus 3.20 p...@freebsd.org | TCP/IP since RFC 956 FreeBSD committer | BSD since 4.3-tahoe Never attribute to malice what can adequately be explained by incompetence. ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
On Wed, Nov 7, 2018 at 9:07 AM Rodney W. Grimes <
free...@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
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
> Reverted, sorry. Turns out that i/o into last_sector+1 is handled
> differently. I'll probably have to use different strategy to properly fail
> out-of-bound ioctl(DIOCGDELETE) or otherwise indicate its result to the
> userland app. To my defense, this patch has been out for 3 weeks on
> freebsd-geom, and I got 0 responses.
Well you have some now, so lets try to sort this out and at least
document what the funny "access one byte past end with zero size
I/O" is about.
A good stratergy when you get no response on a phabricator review
is to poke about it on @current or @arch requesting feedback.
The phabricator notificaiton to potential reviewers is very low.
freebsd-geom is also probably a pretty short list.
> -Max
>
> On Wed, Nov 7, 2018 at 8:06 AM Rodney W. Grimes <
> free...@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.
> >
> > --
> > Rod Grimes
> > rgri...@freebsd.org
> >
> >
--
Rod Grimes rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
Reverted, sorry. Turns out that i/o into last_sector+1 is handled
differently. I'll probably have to use different strategy to properly fail
out-of-bound ioctl(DIOCGDELETE) or otherwise indicate its result to the
userland app. To my defense, this patch has been out for 3 weeks on
freebsd-geom, and I got 0 responses.
-Max
On Wed, Nov 7, 2018 at 8:06 AM Rodney W. Grimes <
free...@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.
>
> --
> Rod Grimes
> rgri...@freebsd.org
>
>
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
> 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.
--
Rod Grimes rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
Li-Wen, thanks for pointing out. I will investigate in the next few hours and follow-up then. Perhaps I need to treat out-of-the bounds BIO_DELETE differently, which was the original issue at hand. -Max On Tue, Nov 6, 2018 at 9:51 PM Rodney W. Grimes < free...@pdx.rh.cn85.dnsmgr.net> wrote: > > Hi Maxim, > > > > There are 5 regression tests failing since this change: > > > > https://ci.freebsd.org/job/FreeBSD-head-amd64-test/9184/testReport/ > > > > sys.geom.class.* > > > > Can you help check them? > > Maybe this should be backed out and looked at more closely? > > > -- > Rod Grimes > rgri...@freebsd.org > > ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
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.
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.
-- Ian
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
Hi Maxim, There are 5 regression tests failing since this change: https://ci.freebsd.org/job/FreeBSD-head-amd64-test/9184/testReport/ sys.geom.class.* Can you help check them? Best, Li-Wen ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
> 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.
> bp->bio_length = 0;
> bp->bio_data = NULL;
> g_io_request(bp, cp);
> ...
> }
>
>
>
> On Tue, Nov 6, 2018 at 8:31 AM Rodney W. Grimes <
> free...@pdx.rh.cn85.dnsmgr.net> wrote:
> >
> > > Author: sobomax
> > > Date: Tue Nov 6 15:55:41 2018
> > > New Revision: 340187
> > > URL: https://svnweb.freebsd.org/changeset/base/340187
> > >
> > > Log:
> > > Don't allow BIO_READ, BIO_WRITE or BIO_DELETE requests that are
> > > fully beyond the end of providers media. The only exception is made
> > > for the zero length transfers which are allowed to be just on the
> > > boundary. Previously, any requests starting on the boundary (i.e. next
> > > byte after the last one) have been allowed to go through.
> > >
> > > No response from: freebsd-geom@, phk
> > > MFC after: 1 month
> > >
> > > Modified:
> > > head/sys/geom/geom_io.c
> > >
> > > Modified: head/sys/geom/geom_io.c
> > >
> ==
> > > --- head/sys/geom/geom_io.c Tue Nov 6 15:52:49 2018(r340186)
> > > +++ head/sys/geom/geom_io.c Tue Nov 6 15:55:41 2018(r340187)
> > > @@ -420,6 +420,8 @@ g_io_check(struct bio *bp)
> > > return (EIO);
> > > if (bp->bio_offset > pp->mediasize)
> > > return (EIO);
> > > + if (bp->bio_offset == pp->mediasize && bp->bio_length > 0)
> > > + return (EIO);
> >
> > Isnt mediasize 0 based, such that any operation at pp->mediasize is
> > technically past the end of the media and should get an EIO no
> > matter how big it is?
> >
> > Who is doing 0 byte operations at pp->mediasize?
> > That code should probably be fixed rather than allowing
> > this special case here.
> >
> > > /* Truncate requests to the end of providers media. */
> > > excess = bp->bio_offset + bp->bio_length;
> > >
> > >
> >
> > --
> > Rod Grimes
> rgri...@freebsd.org
--
Rod Grimes rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
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.
-Max
int
g_io_flush(struct g_consumer *cp)
{
...
bp = g_alloc_bio();
bp->bio_cmd = BIO_FLUSH;
...
bp->bio_offset = cp->provider->mediasize;
bp->bio_length = 0;
bp->bio_data = NULL;
g_io_request(bp, cp);
...
}
On Tue, Nov 6, 2018 at 8:31 AM Rodney W. Grimes <
free...@pdx.rh.cn85.dnsmgr.net> wrote:
>
> > Author: sobomax
> > Date: Tue Nov 6 15:55:41 2018
> > New Revision: 340187
> > URL: https://svnweb.freebsd.org/changeset/base/340187
> >
> > Log:
> > Don't allow BIO_READ, BIO_WRITE or BIO_DELETE requests that are
> > fully beyond the end of providers media. The only exception is made
> > for the zero length transfers which are allowed to be just on the
> > boundary. Previously, any requests starting on the boundary (i.e. next
> > byte after the last one) have been allowed to go through.
> >
> > No response from: freebsd-geom@, phk
> > MFC after: 1 month
> >
> > Modified:
> > head/sys/geom/geom_io.c
> >
> > Modified: head/sys/geom/geom_io.c
> >
==
> > --- head/sys/geom/geom_io.c Tue Nov 6 15:52:49 2018(r340186)
> > +++ head/sys/geom/geom_io.c Tue Nov 6 15:55:41 2018(r340187)
> > @@ -420,6 +420,8 @@ g_io_check(struct bio *bp)
> > return (EIO);
> > if (bp->bio_offset > pp->mediasize)
> > return (EIO);
> > + if (bp->bio_offset == pp->mediasize && bp->bio_length > 0)
> > + return (EIO);
>
> Isnt mediasize 0 based, such that any operation at pp->mediasize is
> technically past the end of the media and should get an EIO no
> matter how big it is?
>
> Who is doing 0 byte operations at pp->mediasize?
> That code should probably be fixed rather than allowing
> this special case here.
>
> > /* Truncate requests to the end of providers media. */
> > excess = bp->bio_offset + bp->bio_length;
> >
> >
>
> --
> Rod Grimes
rgri...@freebsd.org
___
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
Re: svn commit: r340187 - head/sys/geom
> Author: sobomax > Date: Tue Nov 6 15:55:41 2018 > New Revision: 340187 > URL: https://svnweb.freebsd.org/changeset/base/340187 > > Log: > Don't allow BIO_READ, BIO_WRITE or BIO_DELETE requests that are > fully beyond the end of providers media. The only exception is made > for the zero length transfers which are allowed to be just on the > boundary. Previously, any requests starting on the boundary (i.e. next > byte after the last one) have been allowed to go through. > > No response from: freebsd-geom@, phk > MFC after: 1 month > > Modified: > head/sys/geom/geom_io.c > > Modified: head/sys/geom/geom_io.c > == > --- head/sys/geom/geom_io.c Tue Nov 6 15:52:49 2018(r340186) > +++ head/sys/geom/geom_io.c Tue Nov 6 15:55:41 2018(r340187) > @@ -420,6 +420,8 @@ g_io_check(struct bio *bp) > return (EIO); > if (bp->bio_offset > pp->mediasize) > return (EIO); > + if (bp->bio_offset == pp->mediasize && bp->bio_length > 0) > + return (EIO); Isnt mediasize 0 based, such that any operation at pp->mediasize is technically past the end of the media and should get an EIO no matter how big it is? Who is doing 0 byte operations at pp->mediasize? That code should probably be fixed rather than allowing this special case here. > /* Truncate requests to the end of providers media. */ > excess = bp->bio_offset + bp->bio_length; > > -- Rod Grimes rgri...@freebsd.org ___ svn-src-head@freebsd.org mailing list https://lists.freebsd.org/mailman/listinfo/svn-src-head To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
