From 5afc35b69824db01b36418e8f091ffeaeaeb98c9 Mon Sep 17 00:00:00 2001 From: Alexander Motin Date: Tue, 17 Aug 2021 11:44:34 -0400 Subject: [PATCH] Use more atomics in refcounts Use atomic_load_64() for zfs_refcount_count() to prevent torn reads on 32-bit platforms. On 64-bit ones it should not change anything. When built with ZFS_DEBUG but running without tracking enabled use atomics instead of mutexes same as for builds without ZFS_DEBUG. Since rc_tracked can't change live we can check it without lock. Reviewed-by: Brian Behlendorf Reviewed-by: Matthew Ahrens Signed-off-by: Alexander Motin Closes #12420 --- include/sys/zfs_refcount.h | 8 +++--- module/zfs/refcount.c | 51 ++++++++++++++++---------------------- 2 files changed, 26 insertions(+), 33 deletions(-) diff --git a/include/sys/zfs_refcount.h b/include/sys/zfs_refcount.h index fc0cbea1cf..1e6449472e 100644 --- a/include/sys/zfs_refcount.h +++ b/include/sys/zfs_refcount.h @@ -96,8 +96,8 @@ typedef struct refcount { #define zfs_refcount_create_tracked(rc) ((rc)->rc_count = 0) #define zfs_refcount_destroy(rc) ((rc)->rc_count = 0) #define zfs_refcount_destroy_many(rc, number) ((rc)->rc_count = 0) -#define zfs_refcount_is_zero(rc) ((rc)->rc_count == 0) -#define zfs_refcount_count(rc) ((rc)->rc_count) +#define zfs_refcount_is_zero(rc) (zfs_refcount_count(rc) == 0) +#define zfs_refcount_count(rc) atomic_load_64(&(rc)->rc_count) #define zfs_refcount_add(rc, holder) atomic_inc_64_nv(&(rc)->rc_count) #define zfs_refcount_remove(rc, holder) atomic_dec_64_nv(&(rc)->rc_count) #define zfs_refcount_add_many(rc, number, holder) \ @@ -105,13 +105,13 @@ typedef struct refcount { #define zfs_refcount_remove_many(rc, number, holder) \ atomic_add_64_nv(&(rc)->rc_count, -number) #define zfs_refcount_transfer(dst, src) { \ - uint64_t __tmp = (src)->rc_count; \ + uint64_t __tmp = zfs_refcount_count(src); \ atomic_add_64(&(src)->rc_count, -__tmp); \ atomic_add_64(&(dst)->rc_count, __tmp); \ } #define zfs_refcount_transfer_ownership(rc, ch, nh) ((void)0) #define zfs_refcount_transfer_ownership_many(rc, nr, ch, nh) ((void)0) -#define zfs_refcount_held(rc, holder) ((rc)->rc_count > 0) +#define zfs_refcount_held(rc, holder) (zfs_refcount_count(rc) > 0) #define zfs_refcount_not_held(rc, holder) (B_TRUE) #define zfs_refcount_init() diff --git a/module/zfs/refcount.c b/module/zfs/refcount.c index a3877b8d15..354e021d9d 100644 --- a/module/zfs/refcount.c +++ b/module/zfs/refcount.c @@ -112,13 +112,13 @@ zfs_refcount_destroy(zfs_refcount_t *rc) int zfs_refcount_is_zero(zfs_refcount_t *rc) { - return (rc->rc_count == 0); + return (zfs_refcount_count(rc) == 0); } int64_t zfs_refcount_count(zfs_refcount_t *rc) { - return (rc->rc_count); + return (atomic_load_64(&rc->rc_count)); } int64_t @@ -127,15 +127,18 @@ zfs_refcount_add_many(zfs_refcount_t *rc, uint64_t number, const void *holder) reference_t *ref = NULL; int64_t count; - if (rc->rc_tracked) { - ref = kmem_cache_alloc(reference_cache, KM_SLEEP); - ref->ref_holder = holder; - ref->ref_number = number; + if (!rc->rc_tracked) { + count = atomic_add_64_nv(&(rc)->rc_count, number); + ASSERT3U(count, >=, number); + return (count); } + + ref = kmem_cache_alloc(reference_cache, KM_SLEEP); + ref->ref_holder = holder; + ref->ref_number = number; mutex_enter(&rc->rc_mtx); ASSERT3U(rc->rc_count, >=, 0); - if (rc->rc_tracked) - list_insert_head(&rc->rc_list, ref); + list_insert_head(&rc->rc_list, ref); rc->rc_count += number; count = rc->rc_count; mutex_exit(&rc->rc_mtx); @@ -156,16 +159,14 @@ zfs_refcount_remove_many(zfs_refcount_t *rc, uint64_t number, reference_t *ref; int64_t count; - mutex_enter(&rc->rc_mtx); - ASSERT3U(rc->rc_count, >=, number); - if (!rc->rc_tracked) { - rc->rc_count -= number; - count = rc->rc_count; - mutex_exit(&rc->rc_mtx); + count = atomic_add_64_nv(&(rc)->rc_count, -number); + ASSERT3S(count, >=, 0); return (count); } + mutex_enter(&rc->rc_mtx); + ASSERT3U(rc->rc_count, >=, number); for (ref = list_head(&rc->rc_list); ref; ref = list_next(&rc->rc_list, ref)) { if (ref->ref_holder == holder && ref->ref_number == number) { @@ -242,12 +243,10 @@ zfs_refcount_transfer_ownership_many(zfs_refcount_t *rc, uint64_t number, reference_t *ref; boolean_t found = B_FALSE; - mutex_enter(&rc->rc_mtx); - if (!rc->rc_tracked) { - mutex_exit(&rc->rc_mtx); + if (!rc->rc_tracked) return; - } + mutex_enter(&rc->rc_mtx); for (ref = list_head(&rc->rc_list); ref; ref = list_next(&rc->rc_list, ref)) { if (ref->ref_holder == current_holder && @@ -279,13 +278,10 @@ zfs_refcount_held(zfs_refcount_t *rc, const void *holder) { reference_t *ref; + if (!rc->rc_tracked) + return (zfs_refcount_count(rc) > 0); + mutex_enter(&rc->rc_mtx); - - if (!rc->rc_tracked) { - mutex_exit(&rc->rc_mtx); - return (rc->rc_count > 0); - } - for (ref = list_head(&rc->rc_list); ref; ref = list_next(&rc->rc_list, ref)) { if (ref->ref_holder == holder) { @@ -307,13 +303,10 @@ zfs_refcount_not_held(zfs_refcount_t *rc, const void *holder) { reference_t *ref; - mutex_enter(&rc->rc_mtx); - - if (!rc->rc_tracked) { - mutex_exit(&rc->rc_mtx); + if (!rc->rc_tracked) return (B_TRUE); - } + mutex_enter(&rc->rc_mtx); for (ref = list_head(&rc->rc_list); ref; ref = list_next(&rc->rc_list, ref)) { if (ref->ref_holder == holder) {