Re: CVS commit: src/sys/dev/dkwedge
> Date: Tue, 11 Apr 2023 21:50:49 +0200
> From: Michael van Elst
>
> On Wed, Apr 12, 2023 at 01:10:40AM +0700, Robert Elz wrote:
> >
> > | In that state then decrementing dk_rawopens beyond zero will make
> > | dklastclose do the right thing: nothing.
> >
> > Except that if that happens, dk_rawopens will be left == ~0 and the next
> > open attempt will then increment it, back to 0 again, which is almost
> > certainly not what was wanted.
> >
> > dklastclose() used to have code in it like
> >
> > if (...->dk_rawopens > 0) {
> > if (--...->dk_rawopens == 0)
>
>
> Indeed, that part was simplified away.
Correct. I first added the assertion dk_rawopens > 0 in the following
change change because, as I wrote in the commit message:
It is not possible for us to be closing a wedge whose parent is not
open by at least this wedge.
https://mail-index.netbsd.org/source-changes-hg/2022/08/22/msg359438.html
See
https://mail-index.netbsd.org/source-changes-d/2023/04/11/msg013937.html
for more details of why I believe the condition is always true here.
Then, on the grounds that the condition is always true (and asserted
to be so), I removed the conditional in a separate change:
https://mail-index.netbsd.org/source-changes-hg/2022/08/22/msg359439.html
So if you actually hit the assertion, please share diagnostics, or if
you believe there is a way that you could hit the assertion, please
explain how and we can figure out how to address it.
But if not, please put the assertions back, or I will next week.
Re: CVS commit: src/sys/dev/dkwedge
On Wed, Apr 12, 2023 at 01:10:40AM +0700, Robert Elz wrote:
>
> | In that state then decrementing dk_rawopens beyond zero will make
> | dklastclose do the right thing: nothing.
>
> Except that if that happens, dk_rawopens will be left == ~0 and the next
> open attempt will then increment it, back to 0 again, which is almost
> certainly not what was wanted.
>
> dklastclose() used to have code in it like
>
> if (...->dk_rawopens > 0) {
> if (--...->dk_rawopens == 0)
Indeed, that part was simplified away.
--
Michael van Elst
Internet: mlel...@serpens.de
"A potential Snark may lurk in every tree."
Re: CVS commit: src/sys/dev/dkwedge
Date:Tue, 11 Apr 2023 17:21:17 +0200
From:Michael van Elst
Message-ID:
| In that state then decrementing dk_rawopens beyond zero will make
| dklastclose do the right thing: nothing.
Except that if that happens, dk_rawopens will be left == ~0 and the next
open attempt will then increment it, back to 0 again, which is almost
certainly not what was wanted.
dklastclose() used to have code in it like
if (...->dk_rawopens > 0) {
if (--...->dk_rawopens == 0)
so that the -- would never be performed if rawopens was 0 when entered.
| When you want to check for overflows of dk_rawopens (which is difficult
| to overflow as you had to create 2^32 wedges)
It wasn't the overflow that Taylor meant, but this underflow (from 0 -> ~0)
which might be a problem.
(Not really relevant, but it wouldn't be 2^32 wedges, but 2^32 simultaneous
opens of any single wedge, right ... but that's not the real issue, that one
probably can't happen on any normal system, the file table could never get
big enough to allow 2^32 simultaneous opens of everything, let alone one
device).
Either dklastclose() can be called when dk_rawopens == 0 (in which case
the current code is broken) or it cannot, in which case the assertion would
have just verified that.
kre
Re: CVS commit: src/sys/dev/dkwedge
On Tue, Apr 11, 2023 at 09:07:49AM +, Taylor R Campbell wrote: > > (a) how we can legitimately enter a state where the assertions are > violated, and dklastclose is called when the close operation ends in no more openers. There is nothing that guarantees that any open was successful before with the effect that dk_rawopens is > 0 and dk_rawvp is not NULL. In that state then decrementing dk_rawopens beyond zero will make dklastclose do the right thing: nothing. When you want to check for overflows of dk_rawopens (which is difficult to overflow as you had to create 2^32 wedges) you need to watch it being incremented (also temporarily). Crashing after the fact with an assertion in dklastclose doesn't help. -- Michael van Elst Internet: mlel...@serpens.de "A potential Snark may lurk in every tree."
Re: CVS commit: src/sys/dev/dkwedge
> Module Name:src
> Committed By: mlelstv
> Date: Tue Sep 27 17:04:52 UTC 2022
>
> Modified Files:
> src/sys/dev/dkwedge: dk.c
>
> Log Message:
> Remove bogus assertions.
>
> @@ -1221,8 +1221,6 @@ dklastclose(struct dkwedge_softc *sc)
>
> KASSERT(mutex_owned(>sc_dk.dk_openlock));
> KASSERT(mutex_owned(>sc_parent->dk_rawlock));
> - KASSERT(sc->sc_parent->dk_rawopens > 0);
> - KASSERT(sc->sc_parent->dk_rawvp != NULL);
>
> if (--sc->sc_parent->dk_rawopens == 0) {
> struct vnode *const vp = sc->sc_parent->dk_rawvp;
If these assertions are bogus, please add a comment explaining:
(a) how we can legitimately enter a state where the assertions are
violated, and
(b) how this logic is supposed to work when dk_rawopens wraps around
from 0 to UINT_MAX and we try to dk_close_parent(NULL, ...).
Otherwise, please restore these assertions.
Re: CVS commit: src/sys/dev/dkwedge
> >Log Message: >Allow dump to raidframe component which is a wedge. > >N.B. ordinary devices check the partition type only in the xxxsize routine. This is a little scary... christos
Re: CVS commit: src/sys/dev/dkwedge
On Thu, Jun 07, 2012 at 04:15:32PM +, Michael van Elst wrote: Module Name: src Committed By: mlelstv Date: Thu Jun 7 16:15:32 UTC 2012 Modified Files: src/sys/dev/dkwedge: dkwedge_bsdlabel.c Log Message: Use the label's packname to create wedge names instead of the classic device names. Fall back to classic device names when the label has an empty name or the default name 'fictitious'. You probably also want to detect duplicate names... David -- David Laight: da...@l8s.co.uk
