Skip to content

bdev/nvme: fail I/O over to another path on generic errors #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
schmidt-scaled wants to merge 1 commit into
base: nvmf-blocking-from-base
Choose a base branch
from
Open

bdev/nvme: fail I/O over to another path on generic errors #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 100 additions & 2 deletions module/bdev/nvme/bdev_nvme.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,22 @@ struct nvme_bdev_io {
/** Keeps track if first of fused commands was completed */
bool first_fused_completed;

/* How many times the current I/O was retried. */
/* How many times the current I/O was retried on the same path. */
int32_t retry_count;

/* How many times the current I/O was failed over to a different path
* because of a non-path (generic) error. Bounded by the number of
* io_paths so that a deterministic error still terminates. This is
* counted separately from retry_count so that failing over does not
* consume the same-path retry budget (bdev_retry_count).
*/
int32_t path_failover_count;

/* On a failover retry, the io_path that just failed this I/O; the next
* path selection avoids it if another path is available.
*/
struct nvme_io_path *io_path_to_avoid;

/** Expiration value in ticks to retry the current I/O. */
uint64_t retry_ticks;

Expand Down Expand Up @@ -1150,12 +1163,13 @@ static struct nvme_io_path *
_bdev_nvme_find_io_path(struct nvme_bdev_channel *nbdev_ch)
{
struct nvme_io_path *io_path, *start, *non_optimized = NULL;
struct nvme_io_path *avoid = nbdev_ch->io_path_to_avoid;

start = nvme_io_path_get_next(nbdev_ch, nbdev_ch->current_io_path);

io_path = start;
do {
if (spdk_likely(nvme_io_path_is_available(io_path))) {
if (spdk_likely(nvme_io_path_is_available(io_path)) && io_path != avoid) {
switch (io_path->nvme_ns->ana_state) {
case SPDK_NVME_ANA_OPTIMIZED_STATE:
nbdev_ch->current_io_path = io_path;
Expand All @@ -1173,6 +1187,18 @@ _bdev_nvme_find_io_path(struct nvme_bdev_channel *nbdev_ch)
io_path = nvme_io_path_get_next(nbdev_ch, io_path);
} while (io_path != start);

/* On a failover retry we skip the path that just failed the I/O. If it
* turns out to be the only usable path, fall back to it rather than
* failing the I/O outright.
*/
if (non_optimized == NULL && avoid != NULL && nvme_io_path_is_available(avoid)) {
if (nbdev_ch->mp_policy == BDEV_NVME_MP_POLICY_ACTIVE_PASSIVE ||
avoid->nvme_ns->ana_state == SPDK_NVME_ANA_OPTIMIZED_STATE) {
nbdev_ch->current_io_path = avoid;
}
return avoid;
}

if (nbdev_ch->mp_policy == BDEV_NVME_MP_POLICY_ACTIVE_ACTIVE) {
/* We come here only if there is no optimized path. Cache even non_optimized
* path for load balance across multiple non_optimized paths.
Expand Down Expand Up @@ -1284,6 +1310,41 @@ any_io_path_may_become_available(struct nvme_bdev_channel *nbdev_ch)
return false;
}

/* Total number of io_paths in the channel (regardless of current state). Used
* only to bound the number of failover attempts for a single I/O.
*/
static uint32_t
bdev_nvme_num_io_paths(struct nvme_bdev_channel *nbdev_ch)
{
struct nvme_io_path *io_path;
uint32_t num = 0;

STAILQ_FOREACH(io_path, &nbdev_ch->io_path_list, stailq) {
num++;
}

return num;
}

/* Return true if there is an available io_path other than "except". */
static bool
bdev_nvme_another_io_path_is_available(struct nvme_bdev_channel *nbdev_ch,
struct nvme_io_path *except)
{
struct nvme_io_path *io_path;

STAILQ_FOREACH(io_path, &nbdev_ch->io_path_list, stailq) {
if (io_path == except) {
continue;
}
if (nvme_io_path_is_available(io_path)) {
return true;
}
}

return false;
}

static void
bdev_nvme_retry_io(struct nvme_bdev_channel *nbdev_ch, struct spdk_bdev_io *bdev_io)
{
Expand Down Expand Up @@ -1537,6 +1598,34 @@ bdev_nvme_check_retry_io(struct nvme_bdev_io *bio,
return false;
}
*_delay_ms = 0;
} else if (bio->path_failover_count < bdev_nvme_num_io_paths(nbdev_ch) &&
bdev_nvme_another_io_path_is_available(nbdev_ch, io_path)) {
/*
* A non-path (generic) error was returned on a path whose qpair and
* controller still look healthy, so it did not match the path-error
* checks above. In a multipath configuration where each path leads to
* an independent controller, such an error is frequently specific to
* this path/controller while other paths are healthy (e.g. the target
* node is failing but its connection has not been torn down yet).
*
* Fail the I/O over to a different path instead of retrying on the
* same, possibly-dying path. Crucially this is counted against
* path_failover_count, not retry_count, so that dead-end attempts do
* not consume the same-path retry budget (bdev_retry_count) that the
* failover itself also relies on.
*
* The number of failovers is bounded by the number of io_paths and we
* only fail over while a *different* path is available, so a genuinely
* deterministic error (e.g. a media error returned identically by every
* path) still terminates: once every path has been tried the I/O falls
* through to the same-path retry branch below and is eventually
* reported to the upper layer.
*/
bio->path_failover_count++;
bio->io_path_to_avoid = io_path;
bdev_nvme_clear_current_io_path(nbdev_ch);
bio->io_path = NULL;
*_delay_ms = 0;
} else {
bio->retry_count++;

Expand Down Expand Up @@ -1592,6 +1681,8 @@ bdev_nvme_io_complete_nvme_status(struct nvme_bdev_io *bio,

complete:
bio->retry_count = 0;
bio->path_failover_count = 0;
bio->io_path_to_avoid = NULL;
bio->submit_tsc = 0;
bdev_io->u.bdev.accel_sequence = NULL;
__bdev_nvme_io_complete(bdev_io, 0, cpl);
Expand Down Expand Up @@ -1635,6 +1726,8 @@ bdev_nvme_io_complete(struct nvme_bdev_io *bio, int rc)
}

bio->retry_count = 0;
bio->path_failover_count = 0;
bio->io_path_to_avoid = NULL;
bio->submit_tsc = 0;
__bdev_nvme_io_complete(bdev_io, io_status, NULL);
}
Expand Down Expand Up @@ -3455,7 +3548,12 @@ bdev_nvme_submit_request(struct spdk_io_channel *ch, struct spdk_bdev_io *bdev_i
}

spdk_trace_record(TRACE_BDEV_NVME_IO_START, 0, 0, (uintptr_t)nbdev_io, (uintptr_t)bdev_io);
/* Publish the failover skip hint for the duration of this synchronous
* path selection only, then clear it so it cannot affect other I/Os.
*/
nbdev_ch->io_path_to_avoid = nbdev_io->io_path_to_avoid;
nbdev_io->io_path = bdev_nvme_find_io_path(nbdev_ch);
nbdev_ch->io_path_to_avoid = NULL;
if (spdk_unlikely(!nbdev_io->io_path)) {
if (!bdev_nvme_io_type_is_admin(bdev_io->type)) {
bdev_nvme_io_complete(nbdev_io, -ENXIO);
Expand Down
7 changes: 7 additions & 0 deletions module/bdev/nvme/bdev_nvme.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,13 @@ struct nvme_io_path {

struct nvme_bdev_channel {
struct nvme_io_path *current_io_path;
/* Transient hint, set only for the duration of a single (synchronous)
* path selection during a failover retry: the io_path that just failed
* the I/O and should be skipped if any other path is available. It is
* consumed and cleared by _bdev_nvme_find_io_path(). Never dereferenced
* outside that synchronous window.
*/
struct nvme_io_path *io_path_to_avoid;
enum spdk_bdev_nvme_multipath_policy mp_policy;
enum spdk_bdev_nvme_multipath_selector mp_selector;
uint32_t rr_min_io;
Expand Down
Loading