This is a work in progress report for my work on safer use of atomics in ZFS
that was discussed in the August developer meeting.
I took the approach suggested at the meeting of creating a new type that is to
be used for all objects that should be modified atomically.
The type is trivial:
typedef struct zatomic64 {
volatile uint64_t val;
} zatomic64_t;
I also made a decision upfront to use a wrapper type only for 64-bit objects as
32-bit objects should be already safe on all supported platforms where a native
integer size is either 32 or 64 bit.
Then, as suggested, I wrote trivial wrappers for all 64-bit atomic operations
that are used in ZFS code. For example:
static inline void
zatomic_add_64(zatomic64_t *a, uint64_t val)
{
atomic_add_64(&a->val, val);
}
A side note on naming. I simply prepended original function names with 'z'.
Although names like zatomic64_add() could be prettier and more aligned with the
type name.
After that I added three new functions:
- zatomic_load_64 -- safe (with respect to torn values) read of a 64-bit atomic
- zatomic_store_64 -- safe write of a 64-bit atomic
- zatomic_init_64 -- unsafe write of a 64-bit atomic
The last function just sets zatomic64_t.val and it is supposed to be used when
it is known that there are no concurrent accesses. It's pretty redundant the
field can be set directly or the whole object can be initialized with x = { 0 },
but I decided to add it for symmetry.
The first two functions are implemented like this:
static inline uint64_t
zatomic_load_64(zatomic64_t *a)
{
#ifdef __LP64__
return (a->val);
#else
return (zatomic_add_64_nv(a, 0));
#endif
}
static inline void
zatomic_store_64(zatomic64_t *a, uint64_t val)
{
#ifdef __LP64__
a->val = val;
#else
(void)zatomic_swap_64(a, 0);
#endif
}
I am not sure if this was a right way to do it.
Maybe there should be a standard implementation only for 64-bit platforms and
each 32-bit platform should do its own thing?
For example, there is more than one way to get atomic 64-bit loads on x86,
according to this:
https://stackoverflow.com/questions/48046591/how-do-i-atomically-move-a-64bit-value-in-x86-asm
Anyway, I started converting ZFS (/FreeBSD) code to use the new type.
Immediately, I got some problems and some questions.
First, I am quite hesitant about what to do with kstat-s.
I wanted to confine the change to ZFS, but kstat is an external thing (at least
on illumos). For now I decided to leave the kstat-s alone. Especially as I was
not going to change any code that reads the kstat-s.
But this also means that some things like arc_c and arc_p, which are aliases to
kstat value, remain potentially unsafe with respect to torn reads.
I have not converted atomics in the aggsum code yet.
I am actually a little bit confused about why as_lower_bound and as_upper_bound
are manipulated with atomics. All manipulations seems to be done under as_lock.
Maybe I overlooked something...
I converted refcount.h.
That had a bit of unexpected effect on rrwlock_t.
zfs_refcount_t is used for rr_anon_rcount and rr_linked_rcount fields.
The fields are always accessed rr_lock, so their atomicity is not actually
required. I guess that zfs_refcount_t is used for the support of references
with ownership. So, some bits of the rrwlock code access those fields through
the refcount interface, but other (conditional) pieces directly access rc_count
as a simple integer.
So, there are a couple of options here. The direct accesses can be replaced
with refcount accesses, but then we get unnecessary atomic operations in the
fast paths. Or the code can keep directly accessing internals of both
zfs_refcount_t and zatomic64_t. Another option is to create a copy of
zfs_refcount_t that relies on external synchronization (so that it does not use
atomics or any internal locks). But that would be a bit of code duplication.
Yet another hard case is znode's z_size.
It's changed with atomic_cas_64() in a single place, in all other places it's
used as a normal integer. On the one hand, there can be a torn read because of
that atomic operation. On the other hand, it's quite messy to update all uses
of z_size.
So, the work is progressing, but I feel like limiting the scope of the change.
Originally I planned to convert everything to zatomic in one go.
But now I feel that just a few things really need that.
Probably os_obj_next_percpu.
Likely zfs_refcount_t.
Anything else?
I am thinking that maybe a few things could be converted initially and the rest
could be converted on as needed basis.
I don't like having a mix of regular atomics and zatomics, but alas.
Any suggestions, comments, observations, ideas, etc are very welcome!
P.S.
Another thought is creeping in: maybe it's not worth bothering with ZFS on
32-bit platforms.
--
Andriy Gapon
------------------------------------------
openzfs: openzfs-developer
Permalink:
https://openzfs.topicbox.com/groups/developer/T3ee8a81d5f09f2ec-Maf1b71fc5460f9fee01ca4cb
Delivery options: https://openzfs.topicbox.com/groups/developer/subscription
