Skip to content

Commit

Permalink
Prevent race in blkptr_verify against device removal
Browse files Browse the repository at this point in the history 
When we check the vdev of the blkptr in zfs_blkptr_verify, we can run 
into a race condition where that vdev is temporarily unavailable. This 
happens when a device removal operation and the old vdev_t has been 
removed from the array, but the new indirect vdev has not yet been 
inserted.

We hold the spa_config_lock while doing our sensitive verification. 
To ensure that we don't deadlock, we only grab the lock if we don't 
have config_writer held. In addition, I had to const the tags of the 
refcounts and the spa_config_lock arguments.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Serapheim Dimitropoulos <[email protected]>
Signed-off-by: Paul Dagnelie <[email protected]>
Closes #9112
  • Loading branch information
@pcd1193182 @behlendorf
pcd1193182 authored and behlendorf committed Aug 14, 2019
1 parent 8e556c5 commit dc04a8c
Show file tree
Hide file tree
Showing 5 changed files with 35 additions and 28 deletions.
19 changes: 10 additions & 9 deletions include/sys/refcount.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ extern "C" {
#ifdef ZFS_DEBUG
typedef struct reference {
list_node_t ref_link;
void *ref_holder;
const void *ref_holder;
uint64_t ref_number;
uint8_t *ref_removed;
} reference_t;
Expand All @@ -70,16 +70,17 @@ void zfs_refcount_destroy(zfs_refcount_t *);
void zfs_refcount_destroy_many(zfs_refcount_t *, uint64_t);
int zfs_refcount_is_zero(zfs_refcount_t *);
int64_t zfs_refcount_count(zfs_refcount_t *);
int64_t zfs_refcount_add(zfs_refcount_t *, void *);
int64_t zfs_refcount_remove(zfs_refcount_t *, void *);
int64_t zfs_refcount_add_many(zfs_refcount_t *, uint64_t, void *);
int64_t zfs_refcount_remove_many(zfs_refcount_t *, uint64_t, void *);
int64_t zfs_refcount_add(zfs_refcount_t *, const void *);
int64_t zfs_refcount_remove(zfs_refcount_t *, const void *);
int64_t zfs_refcount_add_many(zfs_refcount_t *, uint64_t, const void *);
int64_t zfs_refcount_remove_many(zfs_refcount_t *, uint64_t, const void *);
void zfs_refcount_transfer(zfs_refcount_t *, zfs_refcount_t *);
void zfs_refcount_transfer_ownership(zfs_refcount_t *, void *, void *);
void zfs_refcount_transfer_ownership(zfs_refcount_t *, const void *,
const void *);
void zfs_refcount_transfer_ownership_many(zfs_refcount_t *, uint64_t,
void *, void *);
boolean_t zfs_refcount_held(zfs_refcount_t *, void *);
boolean_t zfs_refcount_not_held(zfs_refcount_t *, void *);
const void *, const void *);
boolean_t zfs_refcount_held(zfs_refcount_t *, const void *);
boolean_t zfs_refcount_not_held(zfs_refcount_t *, const void *);

void zfs_refcount_init(void);
void zfs_refcount_fini(void);
Expand Down
5 changes: 2 additions & 3 deletions include/sys/spa.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1007,8 +1007,8 @@ extern int spa_import_progress_set_state(uint64_t pool_guid,

/* Pool configuration locks */
extern int spa_config_tryenter(spa_t *spa, int locks, void *tag, krw_t rw);
extern void spa_config_enter(spa_t *spa, int locks, void *tag, krw_t rw);
extern void spa_config_exit(spa_t *spa, int locks, void *tag);
extern void spa_config_enter(spa_t *spa, int locks, const void *tag, krw_t rw);
extern void spa_config_exit(spa_t *spa, int locks, const void *tag);
extern int spa_config_held(spa_t *spa, int locks, krw_t rw);

/* Pool vdev add/remove lock */
Expand Down Expand Up @@ -1123,7 +1123,6 @@ extern boolean_t spa_has_checkpoint(spa_t *spa);
extern boolean_t spa_importing_readonly_checkpoint(spa_t *spa);
extern boolean_t spa_suspend_async_destroy(spa_t *spa);
extern uint64_t spa_min_claim_txg(spa_t *spa);
extern void zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp);
extern boolean_t zfs_dva_valid(spa_t *spa, const dva_t *dva,
const blkptr_t *bp);
typedef void (*spa_remap_cb_t)(uint64_t vdev, uint64_t offset, uint64_t size,
Expand Down
19 changes: 10 additions & 9 deletions module/zfs/refcount.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ zfs_refcount_count(zfs_refcount_t *rc)
}

int64_t
zfs_refcount_add_many(zfs_refcount_t *rc, uint64_t number, void *holder)
zfs_refcount_add_many(zfs_refcount_t *rc, uint64_t number, const void *holder)
{
reference_t *ref = NULL;
int64_t count;
Expand All @@ -143,13 +143,14 @@ zfs_refcount_add_many(zfs_refcount_t *rc, uint64_t number, void *holder)
}

int64_t
zfs_refcount_add(zfs_refcount_t *rc, void *holder)
zfs_refcount_add(zfs_refcount_t *rc, const void *holder)
{
return (zfs_refcount_add_many(rc, 1, holder));
}

int64_t
zfs_refcount_remove_many(zfs_refcount_t *rc, uint64_t number, void *holder)
zfs_refcount_remove_many(zfs_refcount_t *rc, uint64_t number,
const void *holder)
{
reference_t *ref;
int64_t count;
Expand Down Expand Up @@ -197,7 +198,7 @@ zfs_refcount_remove_many(zfs_refcount_t *rc, uint64_t number, void *holder)
}

int64_t
zfs_refcount_remove(zfs_refcount_t *rc, void *holder)
zfs_refcount_remove(zfs_refcount_t *rc, const void *holder)
{
return (zfs_refcount_remove_many(rc, 1, holder));
}
Expand Down Expand Up @@ -235,7 +236,7 @@ zfs_refcount_transfer(zfs_refcount_t *dst, zfs_refcount_t *src)

void
zfs_refcount_transfer_ownership_many(zfs_refcount_t *rc, uint64_t number,
void *current_holder, void *new_holder)
const void *current_holder, const void *new_holder)
{
reference_t *ref;
boolean_t found = B_FALSE;
Expand All @@ -260,8 +261,8 @@ zfs_refcount_transfer_ownership_many(zfs_refcount_t *rc, uint64_t number,
}

void
zfs_refcount_transfer_ownership(zfs_refcount_t *rc, void *current_holder,
void *new_holder)
zfs_refcount_transfer_ownership(zfs_refcount_t *rc, const void *current_holder,
const void *new_holder)
{
return (zfs_refcount_transfer_ownership_many(rc, 1, current_holder,
new_holder));
Expand All @@ -273,7 +274,7 @@ zfs_refcount_transfer_ownership(zfs_refcount_t *rc, void *current_holder,
* might be held.
*/
boolean_t
zfs_refcount_held(zfs_refcount_t *rc, void *holder)
zfs_refcount_held(zfs_refcount_t *rc, const void *holder)
{
reference_t *ref;

Expand Down Expand Up @@ -301,7 +302,7 @@ zfs_refcount_held(zfs_refcount_t *rc, void *holder)
* since the reference might not be held.
*/
boolean_t
zfs_refcount_not_held(zfs_refcount_t *rc, void *holder)
zfs_refcount_not_held(zfs_refcount_t *rc, const void *holder)
{
reference_t *ref;

Expand Down
4 changes: 2 additions & 2 deletions module/zfs/spa_misc.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -484,7 +484,7 @@ spa_config_tryenter(spa_t *spa, int locks, void *tag, krw_t rw)
}

void
spa_config_enter(spa_t *spa, int locks, void *tag, krw_t rw)
spa_config_enter(spa_t *spa, int locks, const void *tag, krw_t rw)
{
int wlocks_held = 0;

Expand Down Expand Up @@ -517,7 +517,7 @@ spa_config_enter(spa_t *spa, int locks, void *tag, krw_t rw)
}

void
spa_config_exit(spa_t *spa, int locks, void *tag)
spa_config_exit(spa_t *spa, int locks, const void *tag)
{
for (int i = SCL_LOCKS - 1; i >= 0; i--) {
spa_config_lock_t *scl = &spa->spa_config_lock[i];
Expand Down
16 changes: 11 additions & 5 deletions module/zfs/zio.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -881,8 +881,8 @@ zio_root(spa_t *spa, zio_done_func_t *done, void *private, enum zio_flag flags)
return (zio_null(NULL, spa, NULL, done, private, flags));
}

void
zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp)
static void
zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp, boolean_t config_held)
{
if (!DMU_OT_IS_VALID(BP_GET_TYPE(bp))) {
zfs_panic_recover("blkptr at %p has invalid TYPE %llu",
Expand Down Expand Up @@ -921,6 +921,10 @@ zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp)
if (!spa->spa_trust_config)
return;

if (!config_held)
spa_config_enter(spa, SCL_VDEV, bp, RW_READER);
else
ASSERT(spa_config_held(spa, SCL_VDEV, RW_WRITER));
/*
* Pool-specific checks.
*
Expand Down Expand Up @@ -969,6 +973,8 @@ zfs_blkptr_verify(spa_t *spa, const blkptr_t *bp)
bp, i, (longlong_t)offset);
}
}
if (!config_held)
spa_config_exit(spa, SCL_VDEV, bp);
}

boolean_t
Expand Down Expand Up @@ -1008,7 +1014,7 @@ zio_read(zio_t *pio, spa_t *spa, const blkptr_t *bp,
{
zio_t *zio;

zfs_blkptr_verify(spa, bp);
zfs_blkptr_verify(spa, bp, flags & ZIO_FLAG_CONFIG_WRITER);

zio = zio_create(pio, spa, BP_PHYSICAL_BIRTH(bp), bp,
data, size, size, done, private,
Expand Down Expand Up @@ -1101,7 +1107,7 @@ void
zio_free(spa_t *spa, uint64_t txg, const blkptr_t *bp)
{

zfs_blkptr_verify(spa, bp);
zfs_blkptr_verify(spa, bp, B_FALSE);

/*
* The check for EMBEDDED is a performance optimization. We
Expand Down Expand Up @@ -1171,7 +1177,7 @@ zio_claim(zio_t *pio, spa_t *spa, uint64_t txg, const blkptr_t *bp,
{
zio_t *zio;

zfs_blkptr_verify(spa, bp);
zfs_blkptr_verify(spa, bp, flags & ZIO_FLAG_CONFIG_WRITER);

if (BP_IS_EMBEDDED(bp))
return (zio_null(pio, spa, NULL, NULL, NULL, 0));
Expand Down

0 comments on commit dc04a8c

Please sign in to comment.