Changeset View
Standalone View
sys/geom/geom_vfs.c
Show First 20 Lines • Show All 51 Lines • ▼ Show 20 Lines | |||||
struct g_vfs_softc { | struct g_vfs_softc { | ||||
struct mtx sc_mtx; | struct mtx sc_mtx; | ||||
struct bufobj *sc_bo; | struct bufobj *sc_bo; | ||||
struct g_event *sc_event; | struct g_event *sc_event; | ||||
int sc_active; | int sc_active; | ||||
bool sc_orphaned; | bool sc_orphaned; | ||||
int sc_enxio_active; | int sc_enxio_active; | ||||
int sc_enxio_reported; | |||||
}; | }; | ||||
static struct buf_ops __g_vfs_bufops = { | static struct buf_ops __g_vfs_bufops = { | ||||
.bop_name = "GEOM_VFS", | .bop_name = "GEOM_VFS", | ||||
.bop_write = bufwrite, | .bop_write = bufwrite, | ||||
.bop_strategy = g_vfs_strategy, | .bop_strategy = g_vfs_strategy, | ||||
.bop_sync = bufsync, | .bop_sync = bufsync, | ||||
.bop_bdflush = bufbdflush | .bop_bdflush = bufbdflush | ||||
▲ Show 20 Lines • Show All 76 Lines • ▼ Show 20 Lines | g_vfs_done(struct bio *bip) | ||||
sc = cp->geom->softc; | sc = cp->geom->softc; | ||||
if (bip->bio_error != 0 && bip->bio_error != EOPNOTSUPP) { | if (bip->bio_error != 0 && bip->bio_error != EOPNOTSUPP) { | ||||
if ((bp->b_xflags & BX_CVTENXIO) != 0) { | if ((bp->b_xflags & BX_CVTENXIO) != 0) { | ||||
if (atomic_cmpset_int(&sc->sc_enxio_active, 0, 1)) | if (atomic_cmpset_int(&sc->sc_enxio_active, 0, 1)) | ||||
printf("g_vfs_done(): %s converting all errors to ENXIO\n", | printf("g_vfs_done(): %s converting all errors to ENXIO\n", | ||||
bip->bio_to->name); | bip->bio_to->name); | ||||
} | } | ||||
if (sc->sc_enxio_active) | if (sc->sc_enxio_active) | ||||
bip->bio_error = ENXIO; | bip->bio_error = ENXIO; | ||||
g_print_bio("g_vfs_done():", bip, "error = %d", | if (bip->bio_error != ENXIO || | ||||
kib: I think, if such option is ever present, you should also add a logging of the sc_enxio_active =… | |||||
Done Inline ActionsSo add something like if (sc->sc_enxio_active == 0) g_print("g_vfs_done() converting all future errors to ENXIO for %s", mumble); before we set sc_enxio_active = 1? Or do it conditionally based on g_report_enxio? I think we just want to do that always on the 0->1 transition, independent of this option. There's no other place to do this only once, and UFS, the current only user of CVTENXIO doesn't report anything. You say 'if such an option'. does that mean you'd rather not see an option and just never report ENXIO or are you saying you are just luke warm on this notion? imp: So add something like
```
if (sc->sc_enxio_active == 0)
g_print("g_vfs_done() converting… | |||||
Done Inline ActionsYes, I think that new lines you pasted above are fine, together with existing addition to g_vfs_done() in this review. By 'if such option' I mean that the line above ("... converting ...") becomes quite useful if you also add the existing silencing change already in the review. I do not object to or propose to abandon g_report_enxio. kib: Yes, I think that new lines you pasted above are fine, together with existing addition to… | |||||
bip->bio_error); | atomic_cmpset_int(&sc->sc_enxio_reported, 0, 1)) { | ||||
Not Done Inline ActionsIsn't it spelled atomic_cmpset_int ? kib: Isn't it spelled `atomic_cmpset_int` ? | |||||
g_print_bio("g_vfs_done():", bip, "error = %d%s", | |||||
bip->bio_error, | |||||
bip->bio_error != ENXIO ? "" : | |||||
" supressing further ENXIO"); | |||||
} | |||||
Not Done Inline ActionsIs the check for sc_enxio_reported racy? I mean, could two threads execute this condition and then assign true, simultaneously? If yes, I do not think it is worth fixing, but at least noting it in a comment, for the reader' sake, would be useful. kib: Is the check for sc_enxio_reported racy? I mean, could two threads execute this condition and… | |||||
Done Inline ActionsI'll add a comment. It's only for a printf... The race is tricky to trigger. Normally, we hold the periph lock during most of the completion process, but we drop it just before we call biodone(). This biodone might have a ENXIO return code. The first ENXIO error will trigger posting an AC_LOST_DEVICE to xpt_async_td, which will eventually wind up calling cam_periph_async() which will invalidate the device. That code will grab the periph lock and invalidate all the software queued I/O. These two things might race, but it's unlikely (but not impossible) they'd wind up racing each other through g_vfs_done(). If we lose the race, we'd lose the printf, which isn't the end of the world. A simple cmpset would fix this. This would cause a flood of atomic activity (one per pending I/O when the device fails), but since this is only the error path for a dying device that happens rarely, it wouldn't be terrible. I'm torn on fixing it, though. imp: I'll add a comment. It's only for a printf...
The race is tricky to trigger. Normally, we hold… | |||||
Done Inline Actionssince it's easy enough to fix, I just fixed it (I hope) imp: since it's easy enough to fix, I just fixed it (I hope) | |||||
} | } | ||||
bp->b_error = bip->bio_error; | bp->b_error = bip->bio_error; | ||||
bp->b_ioflags = bip->bio_flags; | bp->b_ioflags = bip->bio_flags; | ||||
if (bip->bio_error) | if (bip->bio_error) | ||||
bp->b_ioflags |= BIO_ERROR; | bp->b_ioflags |= BIO_ERROR; | ||||
bp->b_resid = bp->b_bcount - bip->bio_completed; | bp->b_resid = bp->b_bcount - bip->bio_completed; | ||||
g_destroy_bio(bip); | g_destroy_bio(bip); | ||||
▲ Show 20 Lines • Show All 159 Lines • Show Last 20 Lines |
I think, if such option is ever present, you should also add a logging of the sc_enxio_active = 1 event. In other words, we should clearly report that all errors are converted to ENXIO from now on and then g_report_enxio rules.