Date: Wed, 24 Jan 2018 15:16:17 +0000 (UTC) From: Mark Johnston <markj@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-stable@freebsd.org, svn-src-stable-11@freebsd.org Subject: svn commit: r328334 - in stable/11: sys/geom/mirror tests/sys/geom/class/mirror Message-ID: <201801241516.w0OFGHRB083905@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: markj Date: Wed Jan 24 15:16:17 2018 New Revision: 328334 URL: https://svnweb.freebsd.org/changeset/base/328334 Log: MFC r327779, r327780: Fix handling of read errors during synchronization. Added: stable/11/tests/sys/geom/class/mirror/sync_error.sh - copied unchanged from r327780, head/tests/sys/geom/class/mirror/sync_error.sh Modified: stable/11/sys/geom/mirror/g_mirror.c stable/11/tests/sys/geom/class/mirror/Makefile Directory Properties: stable/11/ (props changed) Modified: stable/11/sys/geom/mirror/g_mirror.c ============================================================================== --- stable/11/sys/geom/mirror/g_mirror.c Wed Jan 24 15:15:18 2018 (r328333) +++ stable/11/sys/geom/mirror/g_mirror.c Wed Jan 24 15:16:17 2018 (r328334) @@ -108,6 +108,8 @@ static int g_mirror_update_disk(struct g_mirror_disk * static void g_mirror_update_device(struct g_mirror_softc *sc, bool force); static void g_mirror_dumpconf(struct sbuf *sb, const char *indent, struct g_geom *gp, struct g_consumer *cp, struct g_provider *pp); +static void g_mirror_sync_reinit(const struct g_mirror_disk *disk, + struct bio *bp, off_t offset); static void g_mirror_sync_stop(struct g_mirror_disk *disk, int type); static void g_mirror_register_request(struct g_mirror_softc *sc, struct bio *bp); @@ -1296,10 +1298,11 @@ g_mirror_sync_request_free(struct g_mirror_disk *disk, /* * Handle synchronization requests. - * Every synchronization request is two-steps process: first, READ request is - * send to active provider and then WRITE request (with read data) to the provider - * being synchronized. When WRITE is finished, new synchronization request is - * send. + * Every synchronization request is a two-step process: first, a read request is + * sent to the mirror provider via the sync consumer. If that request completes + * successfully, it is converted to a write and sent to the disk being + * synchronized. If the write also completes successfully, the synchronization + * offset is advanced and a new read request is submitted. */ static void g_mirror_sync_request(struct g_mirror_softc *sc, struct bio *bp) @@ -1324,13 +1327,16 @@ g_mirror_sync_request(struct g_mirror_softc *sc, struc return; } + sync = &disk->d_sync; + /* * Synchronization request. */ switch (bp->bio_cmd) { - case BIO_READ: - { + case BIO_READ: { + struct g_mirror_disk *d; struct g_consumer *cp; + int readable; KFAIL_POINT_ERROR(DEBUG_FP, g_mirror_sync_request_read, bp->bio_error); @@ -1339,7 +1345,33 @@ g_mirror_sync_request(struct g_mirror_softc *sc, struc G_MIRROR_LOGREQ(0, bp, "Synchronization request failed (error=%d).", bp->bio_error); - g_mirror_sync_request_free(disk, bp); + + /* + * If there's at least one other disk from which we can + * read the block, retry the request. + */ + readable = 0; + LIST_FOREACH(d, &sc->sc_disks, d_next) + if (d->d_state == G_MIRROR_DISK_STATE_ACTIVE && + !(d->d_flags & G_MIRROR_DISK_FLAG_BROKEN)) + readable++; + + /* + * The read error will trigger a syncid bump, so there's + * no need to do that here. + * + * If we can retry the read from another disk, do so. + * Otherwise, all we can do is kick out the new disk. + */ + if (readable == 0) { + g_mirror_sync_request_free(disk, bp); + g_mirror_event_send(disk, + G_MIRROR_DISK_STATE_DISCONNECTED, + G_MIRROR_EVENT_DONTWAIT); + } else { + g_mirror_sync_reinit(disk, bp, bp->bio_offset); + goto retry_read; + } return; } G_MIRROR_LOGREQ(3, bp, @@ -1353,12 +1385,10 @@ g_mirror_sync_request(struct g_mirror_softc *sc, struc cp->index++; g_io_request(bp, cp); return; - } - case BIO_WRITE: - { + } + case BIO_WRITE: { off_t offset; - void *data; - int i, idx; + int i; KFAIL_POINT_ERROR(DEBUG_FP, g_mirror_sync_request_write, bp->bio_error); @@ -1375,7 +1405,6 @@ g_mirror_sync_request(struct g_mirror_softc *sc, struc return; } G_MIRROR_LOGREQ(3, bp, "Synchronization request finished."); - sync = &disk->d_sync; if (sync->ds_offset >= sc->sc_mediasize || sync->ds_consumer == NULL || (sc->sc_flags & G_MIRROR_DEVICE_FLAG_DESTROY) != 0) { @@ -1395,20 +1424,13 @@ g_mirror_sync_request(struct g_mirror_softc *sc, struc } /* Send next synchronization request. */ - data = bp->bio_data; - idx = (int)(uintptr_t)bp->bio_caller1; - g_reset_bio(bp); - bp->bio_cmd = BIO_READ; - bp->bio_offset = sync->ds_offset; - bp->bio_length = MIN(MAXPHYS, sc->sc_mediasize - bp->bio_offset); + g_mirror_sync_reinit(disk, bp, sync->ds_offset); sync->ds_offset += bp->bio_length; - bp->bio_done = g_mirror_sync_done; - bp->bio_data = data; - bp->bio_from = sync->ds_consumer; - bp->bio_to = sc->sc_provider; - bp->bio_caller1 = (void *)(uintptr_t)idx; + +retry_read: G_MIRROR_LOGREQ(3, bp, "Sending synchronization request."); sync->ds_consumer->index++; + /* * Delay the request if it is colliding with a regular request. */ @@ -1434,11 +1456,9 @@ g_mirror_sync_request(struct g_mirror_softc *sc, struc sync->ds_update_ts = time_uptime; } return; - } + } default: - KASSERT(1 == 0, ("Invalid command here: %u (device=%s)", - bp->bio_cmd, sc->sc_name)); - break; + panic("Invalid I/O request %p", bp); } } @@ -2029,15 +2049,39 @@ g_mirror_update_idle(struct g_mirror_softc *sc, struct } static void +g_mirror_sync_reinit(const struct g_mirror_disk *disk, struct bio *bp, + off_t offset) +{ + void *data; + int idx; + + data = bp->bio_data; + idx = (int)(uintptr_t)bp->bio_caller1; + g_reset_bio(bp); + + bp->bio_cmd = BIO_READ; + bp->bio_data = data; + bp->bio_done = g_mirror_sync_done; + bp->bio_from = disk->d_sync.ds_consumer; + bp->bio_to = disk->d_softc->sc_provider; + bp->bio_caller1 = (void *)(uintptr_t)idx; + bp->bio_offset = offset; + bp->bio_length = MIN(MAXPHYS, + disk->d_softc->sc_mediasize - bp->bio_offset); +} + +static void g_mirror_sync_start(struct g_mirror_disk *disk) { struct g_mirror_softc *sc; + struct g_mirror_disk_sync *sync; struct g_consumer *cp; struct bio *bp; int error, i; g_topology_assert_not(); sc = disk->d_softc; + sync = &disk->d_sync; sx_assert(&sc->sc_lock, SX_LOCKED); KASSERT(disk->d_state == G_MIRROR_DISK_STATE_SYNCHRONIZING, @@ -2063,54 +2107,48 @@ g_mirror_sync_start(struct g_mirror_disk *disk) g_mirror_get_diskname(disk)); if ((sc->sc_flags & G_MIRROR_DEVICE_FLAG_NOFAILSYNC) == 0) disk->d_flags |= G_MIRROR_DISK_FLAG_DIRTY; - KASSERT(disk->d_sync.ds_consumer == NULL, + KASSERT(sync->ds_consumer == NULL, ("Sync consumer already exists (device=%s, disk=%s).", sc->sc_name, g_mirror_get_diskname(disk))); - disk->d_sync.ds_consumer = cp; - disk->d_sync.ds_consumer->private = disk; - disk->d_sync.ds_consumer->index = 0; + sync->ds_consumer = cp; + sync->ds_consumer->private = disk; + sync->ds_consumer->index = 0; /* * Allocate memory for synchronization bios and initialize them. */ - disk->d_sync.ds_bios = malloc(sizeof(struct bio *) * g_mirror_syncreqs, + sync->ds_bios = malloc(sizeof(struct bio *) * g_mirror_syncreqs, M_MIRROR, M_WAITOK); for (i = 0; i < g_mirror_syncreqs; i++) { bp = g_alloc_bio(); - disk->d_sync.ds_bios[i] = bp; - bp->bio_parent = NULL; - bp->bio_cmd = BIO_READ; + sync->ds_bios[i] = bp; + bp->bio_data = malloc(MAXPHYS, M_MIRROR, M_WAITOK); - bp->bio_cflags = 0; - bp->bio_offset = disk->d_sync.ds_offset; - bp->bio_length = MIN(MAXPHYS, sc->sc_mediasize - bp->bio_offset); - disk->d_sync.ds_offset += bp->bio_length; - bp->bio_done = g_mirror_sync_done; - bp->bio_from = disk->d_sync.ds_consumer; - bp->bio_to = sc->sc_provider; bp->bio_caller1 = (void *)(uintptr_t)i; + g_mirror_sync_reinit(disk, bp, sync->ds_offset); + sync->ds_offset += bp->bio_length; } /* Increase the number of disks in SYNCHRONIZING state. */ sc->sc_sync.ds_ndisks++; /* Set the number of in-flight synchronization requests. */ - disk->d_sync.ds_inflight = g_mirror_syncreqs; + sync->ds_inflight = g_mirror_syncreqs; /* * Fire off first synchronization requests. */ for (i = 0; i < g_mirror_syncreqs; i++) { - bp = disk->d_sync.ds_bios[i]; + bp = sync->ds_bios[i]; G_MIRROR_LOGREQ(3, bp, "Sending synchronization request."); - disk->d_sync.ds_consumer->index++; + sync->ds_consumer->index++; /* * Delay the request if it is colliding with a regular request. */ if (g_mirror_regular_collision(sc, bp)) g_mirror_sync_delay(sc, bp); else - g_io_request(bp, disk->d_sync.ds_consumer); + g_io_request(bp, sync->ds_consumer); } } Modified: stable/11/tests/sys/geom/class/mirror/Makefile ============================================================================== --- stable/11/tests/sys/geom/class/mirror/Makefile Wed Jan 24 15:15:18 2018 (r328333) +++ stable/11/tests/sys/geom/class/mirror/Makefile Wed Jan 24 15:16:17 2018 (r328334) @@ -18,6 +18,8 @@ TAP_TESTS_SH+= 11_test TAP_TESTS_SH+= 12_test TAP_TESTS_SH+= 13_test +ATF_TESTS_SH+= sync_error + ${PACKAGE}FILES+= conf.sh .for t in ${TAP_TESTS_SH} Copied: stable/11/tests/sys/geom/class/mirror/sync_error.sh (from r327780, head/tests/sys/geom/class/mirror/sync_error.sh) ============================================================================== --- /dev/null 00:00:00 1970 (empty, because file is newly added) +++ stable/11/tests/sys/geom/class/mirror/sync_error.sh Wed Jan 24 15:16:17 2018 (r328334, copy of r327780, head/tests/sys/geom/class/mirror/sync_error.sh) @@ -0,0 +1,110 @@ +# $FreeBSD$ + +REG_READ_FP=debug.fail_point.g_mirror_regular_request_read + +atf_test_case sync_read_error_2_disks cleanup +sync_read_error_2_disks_head() +{ + atf_set "descr" \ + "Ensure that we properly handle read errors during synchronization." + atf_set "require.user" "root" +} +sync_read_error_2_disks_body() +{ + . $(atf_get_srcdir)/conf.sh + + f1=$(mktemp ${base}.XXXXXX) + f2=$(mktemp ${base}.XXXXXX) + + atf_check dd if=/dev/zero bs=1M count=32 of=$f1 status=none + atf_check truncate -s 32M $f2 + + md1=$(attach_md -t vnode -f ${f1}) + md2=$(attach_md -t vnode -f ${f2}) + + atf_check gmirror label $name $md1 + devwait + + atf_check -s exit:0 -e empty -o not-empty sysctl ${REG_READ_FP}='1*return(5)' + + # If a read error occurs while synchronizing and the mirror contains + # a single active disk, gmirror has no choice but to fail the + # synchronization and kick the new disk out of the mirror. + atf_check gmirror insert $name $md2 + sleep 0.1 + syncwait + atf_check [ $(gmirror status -s $name | wc -l) -eq 1 ] + atf_check -s exit:0 -o match:"DEGRADED $md1 \(ACTIVE\)" \ + gmirror status -s $name +} +sync_read_error_2_disks_cleanup() +{ + . $(atf_get_srcdir)/conf.sh + + atf_check -s exit:0 -e empty -o not-empty sysctl ${REG_READ_FP}='off' + gmirror_test_cleanup +} + +atf_test_case sync_read_error_3_disks cleanup +sync_read_error_3_disks_head() +{ + atf_set "descr" \ + "Ensure that we properly handle read errors during synchronization." + atf_set "require.user" "root" +} +sync_read_error_3_disks_body() +{ + . $(atf_get_srcdir)/conf.sh + + f1=$(mktemp ${base}.XXXXXX) + f2=$(mktemp ${base}.XXXXXX) + f3=$(mktemp ${base}.XXXXXX) + + atf_check dd if=/dev/random bs=1M count=32 of=$f1 status=none + atf_check truncate -s 32M $f2 + atf_check truncate -s 32M $f3 + + md1=$(attach_md -t vnode -f ${f1}) + md2=$(attach_md -t vnode -f ${f2}) + md3=$(attach_md -t vnode -f ${f3}) + + atf_check gmirror label $name $md1 + devwait + + atf_check gmirror insert $name $md2 + syncwait + + atf_check -s exit:0 -e empty -o not-empty sysctl ${REG_READ_FP}='1*return(5)' + + # If a read error occurs while synchronizing a new disk, and we have + # multiple active disks, we retry the read after an error. The disk + # which returned the read error is kicked out of the mirror. + atf_check gmirror insert $name $md3 + syncwait + atf_check [ $(gmirror status -s $name | wc -l) -eq 2 ] + atf_check -s exit:0 -o match:"DEGRADED $md3 \(ACTIVE\)" \ + gmirror status -s $name + + # Make sure that the two active disks are identical. Destroy the + # mirror first so that the metadata sectors are wiped. + if $(gmirror status -s $name | grep -q $md1); then + active=$md1 + else + active=$md2 + fi + atf_check gmirror destroy $name + atf_check cmp /dev/$active /dev/$md3 +} +sync_read_error_3_disks_cleanup() +{ + . $(atf_get_srcdir)/conf.sh + + atf_check -s exit:0 -e empty -o not-empty sysctl ${REG_READ_FP}='off' + gmirror_test_cleanup +} + +atf_init_test_cases() +{ + atf_add_test_case sync_read_error_2_disks + atf_add_test_case sync_read_error_3_disks +}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201801241516.w0OFGHRB083905>