Commit Graph

196 Commits

Author SHA1 Message Date
cem
66fb4c422a gmirror: Relocate DEVICE_FLAGS to adjacent lines
gmirror's sc_flags is shared between some on-disk state and some runtime
only state.  There's no real reason for that and they could probably be
split up.  Until they are, locate all of the flags for the same field
nearby each other in the source, for clarity.

No functional change.

Sponsored by:	Dell EMC Isilon
2019-01-23 16:44:21 +00:00
markj
b7a9839424 Use g_handleattr() to reply to GEOM::candelete queries.
g_handleattr() fills out bp->bio_completed; otherwise, g_getattr()
returns an error in response to the query.  This caused BIO_DELETE
support to not be propagated through stacked configurations, e.g.,
a gconcat of gmirror volumes would not handle BIO_DELETE even when
the gmirrors do.  g_io_getattr() was not affected by the problem.

PR:		232676
Reported and tested by:	noah.bergbauer@tum.de
MFC after:	1 week
2019-01-02 15:52:16 +00:00
cem
89e84f5e34 gmirror: Remove a last-minute INVARIANTS breakage in r341840
I mistakenly added a lock assertion to this routine at the last minute
without confirming it was held during g_mirror_create.  It isn't (it isn't
even initialized yet).  Mea culpa.  Access is exclusive in both callers,
just not always by that particular lock.

Reported by:	lwhsu
X-MFC-With:	r341840, r341674
2018-12-12 18:13:56 +00:00
cem
25cda747b0 gmirror: Fix a bug introduced in r341674
r341674 inadvertently introduced a bug where newer mirror components being
tasted would clear the high sc_flags that are not controlled by component
metadata, such as G_MIRROR_DEVICE_FLAG_TASTING.  This could plausibly expose
a small window of time during STARTING where device destruction might race
with mirror component addition, probably resulting in a crash.

Reviewed by:	markj
X-MFC-With:	r341674
Differential Revision:	https://reviews.freebsd.org/D18521
2018-12-12 05:48:27 +00:00
cem
42d84ef531 gmirror: Evaluate mirror components against newest metadata copy
Re-apply r341665 with format strings fixed.

If we happen to taste a stale mirror component first, don't reject valid,
newer components that have differing metadata from the stale component
(during STARTING).  Instead, update our view of the most recent metadata as
we taste components.

Like mediasize beforehand, remove some checks from g_mirror_check_metadata
which would evict valid components due to metadata that can change over a
mirror's lifetime.  g_mirror_check_metadata is invoked long before we check
genid/syncid and decide which component(s) are newest and whether or not we
have quorum.

Before checking if we can enter RUNNING (i.e., we have quorum) after a NEW
component is added, first remove any known stale or inconsistent disks from
the mirrorset, rather than removing them *after* deciding we have quorum.
Check if we have quorum after removing these components.

Additionally, add a knob, kern.geom.mirror.launch_mirror_before_timeout, to
force gmirrors to wait out the full timeout (kern.geom.mirror.timeout)
before transitioning from STARTING to RUNNING.  This is a kludge to help
ensure all eligible, boot-time available mirror components are tasted before
RUNNING a gmirror.

Add a basic test case for STARTING -> RUNNING startup behavior around stale
genids.

PR:		232671, 232835
Submitted by:	Cindy Yang <cyang AT isilon.com> (previous version)
Reviewed by:	markj (kernel portions)
Discussed with:	asomers, Cindy Yang
Tested by:	pho
Sponsored by:	Dell EMC Isilon
Differential Revision:	https://reviews.freebsd.org/D18062
2018-12-07 02:44:04 +00:00
cem
a6f65b4008 Revert r341665 due to tinderbox breakage
I didn't notice that some format strings were non-portable.  Will fix and
re-commit later.
2018-12-07 00:47:05 +00:00
cem
0b3fcef470 gmirror: Evaluate mirror components against newest metadata copy
If we happen to taste a stale mirror component first, don't reject valid,
newer components that have differing metadata from the stale component
(during STARTING).  Instead, update our view of the most recent metadata as
we taste components.

Like mediasize beforehand, remove some checks from g_mirror_check_metadata
which would evict valid components due to metadata that can change over a
mirror's lifetime.  g_mirror_check_metadata is invoked long before we check
genid/syncid and decide which component(s) are newest and whether or not we
have quorum.

Before checking if we can enter RUNNING (i.e., we have quorum) after a NEW
component is added, first remove any known stale or inconsistent disks from
the mirrorset, rather than removing them *after* deciding we have quorum.
Check if we have quorum after removing these components.

Additionally, add a knob, kern.geom.mirror.launch_mirror_before_timeout, to
force gmirrors to wait out the full timeout (kern.geom.mirror.timeout)
before transitioning from STARTING to RUNNING.  This is a kludge to help
ensure all eligible, boot-time available mirror components are tasted before
RUNNING a gmirror.

When we are instructed to forget mirror components, bump the generation id
to avoid confusion with such stale components later.

Add a basic test case for STARTING -> RUNNING startup behavior around stale
genids.

PR:		232671, 232835
Submitted by:	Cindy Yang <cyang AT isilon.com> (previous version)
Reviewed by:	markj (kernel portions)
Discussed with:	asomers, Cindy Yang
Tested by:	pho
Sponsored by:	Dell EMC Isilon
Differential Revision:	https://reviews.freebsd.org/D18062
2018-12-06 23:55:39 +00:00
markj
7f301bacf9 Remove a redundant assertion.
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2018-05-06 00:05:03 +00:00
markj
3579270d7e Avoid dropping the topology lock in gmirror's dumpconf implementation.
Doing so introduces races which can lead to a use-after-free when
grabbing a snapshot of the GEOM mesh.

To ensure that a mirror's disk list remains stable, change its locking
protocol: both the softc lock and the topology lock are now required
to modify the list, so either lock is sufficient for traversal.

Tested by:	pho
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2018-05-06 00:03:24 +00:00
kevans
dd6f2f2c8d Annotate geom modules with MODULE_VERSION
GEOM ELI may double ask the password during boot. Once at loader time, and
once at init time.

This happens due a module loading bug. By default GEOM ELI caches the
password in the kernel, but without the MODULE_VERSION annotation, the
kernel loads over the kernel module, even if the GEOM ELI was compiled into
the kernel. In this case, the newly loaded module
purges/invalidates/overwrites the GEOM ELI's password cache, which causes
the double asking.

MFC Note: There's a pc98 component to the original submission that is
omitted here due to pc98 removal in head. This part will need to be revived
upon MFC.

Reviewed by:	imp
Submitted by:	op
Obtained from:	opBSD
MFC after:	1 week
Differential Revision:	https://reviews.freebsd.org/D14992
2018-04-10 19:18:16 +00:00
markj
1899f7330d Simplify synchronization read error handling.
Since synchronization reads are performed by submitting a request to
the external mirror provider, we know that the request returns with an
error only when gmirror was unable to read a copy of the block from any
mirror. Thus, there is no need to retry the request from the
synchronization error handler.

Tested by:	pho
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2018-02-06 16:02:33 +00:00
markj
652ed11da6 Fix handling of read errors during mirror synchronization.
We would previously just free the request BIO, which would either cause
the disk to stay stuck in the SYNCHRONIZING state, or result in
synchronization completing without having copied the block which
returned an error.

With this change, if the disk which returned an error is the only active
disk in the mirror, the synchronizing disk is kicked out. Otherwise, the
read is retried.

Reported and tested by:	pho (previous version)
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2018-01-10 19:37:21 +00:00
markj
a2d6c6c5ac Clarify the use of the gmirror flag mask constants.
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2018-01-10 15:21:36 +00:00
markj
fa95b8c054 Avoid referencing a possibly freed consumer after r327496.
g_mirror_regular_request() may free the gmirror consumer for a disk
if that disk is being disconnected, after which we must not dereference
the consumer pointer.

CID:		1384280
X-MFC with:	r327496
2018-01-10 05:06:21 +00:00
markj
ab53928d62 Sort and remove unneeded includes.
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2018-01-08 15:56:40 +00:00
markj
807855890b Release the queue lock before restarting the worker loop.
Reported and tested by:	pho
MFC after:	3 days
Sponsored by:	Dell EMC Isilon
2018-01-08 15:41:49 +00:00
markj
6266e5e4ec Fix some I/O ordering issues in gmirror.
- BIO_FLUSH requests were dispatched to the disks directly from
  g_mirror_start() rather than going through the mirror's I/O request
  queue, so they could have been reordered with preceding writes.
  Address this by processing such requests from the queue, avoiding
  direct dispatch.
- Handling for collisions with synchronization requests was too
  fine-grained and could cause reordering of writes. In particular,
  BIO_ORDERED was not being honoured. Address this by effectively
  freezing the request queue any time a collision with a synchronization
  request occurs. The queue is unfrozen once the collision with the
  first frozen request is over.
- The above-mentioned collision handling allowed reads to jump ahead
  of writes to the same offset. Address this by freezing all request
  types when a collision occurs, not just BIO_WRITEs and BIO_DELETEs.

Also add some more fail points for use in testing error handling.

Reviewed by:	imp
MFC after:	3 weeks
Sponsored by:	Dell EMC Isilon
Differential Revision:	https://reviews.freebsd.org/D13559
2018-01-02 18:11:54 +00:00
markj
6afde2097a Avoid using bioq_* in gmirror.
gmirror does not perform any sorting of I/O requests, so the bioq API
doesn't provide any advantages over plain TAILQs. The API also does not
provide operations needed by an upcoming change.

No functional change intended. The diff shrinks the geom_mirror.ko
text and the gmirror softc slightly.

Tested by:	pho (part of a larger patch)
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2017-12-19 17:13:04 +00:00
markj
76756e3bf3 Give a couple of predication functions a bool return type.
No functional change intended.

MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2017-12-15 19:14:21 +00:00
markj
da31570b57 Typo.
MFC after:	1 week
2017-12-15 19:03:03 +00:00
markj
48eee215b9 Address a possible lost wakeup for gmirror events.
g_mirror_event_send() acquires the I/O queue lock to deliver a wakeup
to the worker thread, and this is done after enqueuing the event.
So it's sufficient to check the event queue before atomically releasing
the queue lock and going to sleep.

MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2017-12-12 17:29:34 +00:00
markj
1f07d15939 Give g_mirror_event_get() a more accurate name.
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2017-12-12 17:25:25 +00:00
markj
fd47853c2a Decrement sc_writes when BIO_DELETE requests complete.
Otherwise a gmirror that has received a BIO_DELETE request will never be
marked clean (unless sc_writes overflows).

MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2017-12-12 17:24:30 +00:00
markj
66ff112aae Update gmirror metadata less frequently when synchronizing.
We periodically record synchronization progress in the metadata
block of the disk being synchronized; this allows an interrupted
synchronization to be resumed. However, the frequency of these
updates heavily pessimized synchronization time on some media. This
change modifies gmirror to update metadata based on a time period,
and adds a sysctl to control that period. The default value results
in a much lower update frequency and increases the completion time
for an interrupted rebuild only marginally.

Reported by:	Andre Albsmeier <andre@fbsd.e4m.org>
MFC after:	3 weeks
2017-11-30 20:36:29 +00:00
pfg
a82e3a8b24 sys/geom: adoption of SPDX licensing ID tags.
Mainly focus on files that use BSD 2-Clause license, however the tool I
was using misidentified many licenses so this was mostly a manual - error
prone - task.

The Software Package Data Exchange (SPDX) group provides a specification
to make it easier for automated tools to detect and summarize well known
opensource licenses. We are gradually adopting the specification, noting
that the tags are considered only advisory and do not, in any way,
superceed or replace the license texts.
2017-11-27 15:17:37 +00:00
markj
36a1a14004 Allow kern.geom.mirror.debug to be negative.
A negative value can be used to suppress all prints from the gmirror
kernel code, which can be useful when attempting to trigger race
conditions using stress tests.

MFC after:	1 week
2017-11-23 14:07:52 +00:00
markj
7dbd75884a Fix a lock leak in g_mirror_destroy().
g_mirror_destroy() is supposed to unlock the softc before indicating
success, but it wasn't doing so if the caller raced with another
thread destroying the mirror.

MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2017-10-27 17:05:14 +00:00
avg
78c53bec48 gmirror: treat ENXIO as disk disconnect, not media error
In theory, all data access errors mean that a member is out of sync
at most.  But they were treated as more serious errors to avoid the
situation where a flaky disk gets repeatedly disconnected, re-synchronized,
reconnected and then disconnected again.

ENXIO is a special error that means that the member disk disappeared,
so it should get the same handling as the GEOM orphaning event.
There is a better chance that when the disk is reconnected, it will be
a good member again.

When ENXIO happens on a read we use the exisiting G_MIRROR_BUMP_SYNCID
mechanism which means that the mirror's syncid is increased as soon
as there is a write to the mirror.  That's because no data has got out
of sync yet, but the problematic memeber is disconnected, so the future
write will make it stale.

When ENXIO happens on a write we use a new G_MIRROR_BUMP_SYNCID_NOW
mechanism which means that we update the mirror metadata as soon as
possible because the problematic memeber is already behind.

Reviewed by:	markj, imp
MFC after:	3 weeks
Differential Revision: https://reviews.freebsd.org/D9463
2017-09-15 13:57:08 +00:00
markj
67a31ca9fc Synchronize unclean mirrors before adding them to a running gmirror.
During gmirror startup, if component mirrors are found to be dirty as is
typical after a system crash, the mirrors are synchronized to the mirror
with highest priority. However if a gmirror starts without all of its
mirrors present, for example because of some transient delays during
tasting, the remaining mirrors must be synchronized before they may become
active.

MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-05-02 23:29:42 +00:00
markj
cf195e3f52 Rename two gmirror state flags to make their meanings slightly clearer.
No functional change.

MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-04-14 17:13:57 +00:00
markj
61b9e5a5da Don't set the mirror GEOM softc to NULL in g_mirror_destroy().
At this point we have not rendezvous'ed with the mirror worker thread, and
I/O may still be in flight. Various I/O completion paths expect to be able
to obtain a reference to the mirror softc from the GEOM, so setting it to
NULL may result in various NULL pointer dereferences if the mirror is
stopped with -f or the kernel is shut down while a mirror is
synchronizing. The worker thread will clear the softc pointer before
exiting.

Tested by:	pho
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-04-14 17:08:37 +00:00
markj
63b619ac47 Check for a provider error before enqueuing mirror I/O.
We are otherwise susceptible to a race with a concurrent teardown of the
mirror provider, causing the I/O to be left uncompleted after the mirror
started withering.

Tested by:	pho
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-04-14 17:03:32 +00:00
markj
cc56dafb0b Stop mirror synchronization before draining the I/O queue.
Regular I/O requests may be blocked by concurrent synchronization requests
targeted to the same LBAs, in which case they are moved to a holding queue
until the conflicting I/O completes. We therefore want to stop
synchronization before completing pending I/O in g_mirror_destroy_provider()
since this ensures that blocked I/O requests are completed as well.

Tested by:	pho
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-04-14 16:54:50 +00:00
markj
cb88fec38a Handle NULL entries in gmirror disk ds_bios arrays.
Entries may be removed and freed if an I/O error occurs during mirror
synchronization, so we cannot assume that all entries of ds_bios are
valid.

Also ensure that a synchronization BIO's array index is preserved after
a successful write.

Reported and tested by:	pho
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-04-10 17:15:59 +00:00
markj
15ebdcc4bf Avoid sleeping when the mirror I/O queue is non-empty.
A request may be queued while the queue lock is dropped when the mirror is
being destroyed. The corresponding wakeup would be lost, possibly resulting
in an apparent hang of the mirror worker thread.

Tested by:	pho (part of a larger patch)
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2017-03-29 19:39:07 +00:00
markj
f1678a8682 Remove an unneeded g_mirror_destroy_provider() call.
The worker thread will destroy the mirror provider as part of its teardown
sequence. The call made sense in the initial revision of gmirror, but
became unnecessary in r137248.

Tested by:	pho (part of a larger diff)
MFC afteR:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-03-29 19:30:22 +00:00
markj
4740c5564a Refine r301173 a bit.
- Don't execute any of g_mirror_shutdown_post_sync() when panicking. We
  cannot safely idle the mirror or stop synchronization in that state, and
  the current attempts to do so complicate debugging of gmirror itself.
- Check for a non-NULL panicstr instead of using SCHEDULER_STOPPED(). The
  latter was added for use in the locking primitives.

Reviewed by:	mav, pjd
MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2017-03-27 16:25:58 +00:00
mav
7acb1dbb48 Add gmirror create subcommand, alike to gstripe, gconcat, etc.
It is quite specific mode of operation without storing on-disk metadata.
It can be useful in some cases in combination with some external control
tools handling mirror creation and disks hot-plug.

MFC after:	2 weeks
Sponsored by:	iXsystems, Inc.
2016-11-30 09:27:08 +00:00
mav
e6c44edd72 Use providergone method to cover race between destroy and g_access().
Reviewed by:	markj
MFC after:	2 weeks
Sponsored by:	iXsystems, Inc.
2016-11-13 03:56:26 +00:00
markj
645711f058 gmirror: Add a subroutine to free synchronization BIOs.
This addresses a memory leak that occurs upon an I/O error during a mirror
synchronization.

MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2016-10-20 23:08:40 +00:00
markj
18cd64ee0d gmirror: Release pending regular requests when synchronization stops.
Normally gmirror allows colliding requests to proceed whenever a
synchronization request completes and advances to the next offset. However
if an I/O request collides with one of the final g_mirror_syncreqs, nothing
releases it once synchronization completes, resulting in an apparent I/O
hang. The same problem can occur if synchronization is aborted by an
I/O error. Therefore, be sure to requeue pending requests when
mirror synchronization is stopped for any reason.

While here, remove some dead code from g_mirror_regular_release().

MFC after:	2 weeks
Sponsored by:	Dell EMC Isilon
2016-10-20 23:02:30 +00:00
mav
d1e2d58d18 Fix possible geom destruction before final provider close.
Introduce internal counter to track opens.  Using provider's counters is
not very successfull after calling g_wither_provider().

MFC after:	2 weeks
Sponsored by:	iXsystems, Inc.
2016-10-06 15:20:05 +00:00
markj
8c59e4b664 gmirror: Write an updated syncid before queuing writes.
When a syncid bump is pending, any write to the mirror results in the
updated syncid being written to each component's metadata block. However,
the update was only being performed after the writes to the mirror
componenents were queued. Instead, synchronously update the metadata block
first.

MFC after:	3 weeks
Sponsored by:	Dell EMC Isilon
2016-10-06 00:13:55 +00:00
markj
780b5bd00e gmirror: Bump the syncid if broken disks are found during startup.
Consider a mirror with two components, m1 and m2. Suppose a hardware error
results in the removal of m2, with m1's genid bumped. Suppose further that
a replacement mirror component m3 is created and synchronized, after which
the system is shut down uncleanly. During a subsequent bootup, if gmirror
tastes m1 and m2 first, m2 will be removed from the mirror because it is
broken, but the mirror will be started without bumping the syncid on m1
because all elements of the mirror are accounted for. Then m3 will be
added to the already-running mirror with the same syncid as m1, so the
components will not be synchronized despite the unclean shutdown.

Handle this scenario by bumping the syncid of healthy components if any
broken mirrors are discovered during mirror startup.

MFC after:	3 weeks
Sponsored by:	Dell EMC Isilon
2016-10-06 00:05:45 +00:00
markj
33afbd593e gmirror: Use bool instead of boolean_t.
MFC after:	1 week
Sponsored by:	Dell EMC Isilon
2016-10-05 23:55:01 +00:00
mav
990a95381b Use g_wither_provider() where applicable.
It is just a helper function combining G_PF_WITHER setting with
g_orphan_provider().
2016-09-23 21:29:40 +00:00
markj
866957573f Don't treat an error from g_mirror_clear_metadata() as fatal.
Such errors can occur as the result of a write error or because the disk
backing the mirror element was removed. They result in a generation ID bump
on all active elements of the mirror, so we can safely disconnect the mirror
component rather than destroy it.

MFC after:	2 weeks
Sponsored by:	EMC / Isilon Storage Division
Differential Revision:	https://reviews.freebsd.org/D7750
2016-09-06 23:42:59 +00:00
markj
0d7f42693b Add some fail points to gmirror.
These are useful for testing changes to I/O error handling, and for
reproducing existing bugs in a controlled manner. The fail points are

    g_mirror_regular_request_read
    g_mirror_regular_request_write
    g_mirror_sync_request_read
    g_mirror_sync_request_write
    g_mirror_metadata_write

They all effectively allow one to inject an error value into the bio_error
field of a corresponding BIO request as it is being completed.

MFC after:	2 weeks
Sponsored by:	EMC / Isilon Storage Division
2016-09-06 23:35:48 +00:00
markj
32d16ca9fb Move some gmirror metadata update messages to a higher debug level.
These can be printed quite frequently from a mostly-idle mirror, cluttering
the console.

MFC after:	1 week
2016-07-14 00:40:24 +00:00
markj
a8b2bf9492 Do not complete pending gmirror BIOs when tearing down the provider.
This will result in lock recursion and is more generally incorrect since
the completion handlers will just reinsert the BIOs into the queue we're
trying to drain.

Reviewed by:	imp, ngie
Approved by:	re (gjb)
MFC after:	3 weeks
Sponsored by:	EMC / Isilon Storage Division
Differential Revision:	https://reviews.freebsd.org/D6908
2016-06-22 21:00:28 +00:00