Skip to content
This repository has been archived by the owner on Nov 7, 2019. It is now read-only.

7968 multi-threaded spa_sync #138

Closed
wants to merge 2 commits into from
Closed

7968 multi-threaded spa_sync #138

wants to merge 2 commits into from

Conversation

ahrens
Copy link
Member

@ahrens ahrens commented Jun 27, 2016

spa_sync() iterates over all the dirty dnodes and processes each of them by calling dnode_sync(). If there are many dirty dnodes (e.g. because we created or removed a lot of files), the single thread of spa_sync() calling dnode_sync() can become a bottleneck. Additionally, if many dnodes are dirtied concurrently in open context (e.g. due to concurrent file creation), the os_lock will experience lock contention via dnode_setdirty().

The solution is to track dirty dnodes on a multilist_t, and for spa_sync() to use separate threads to process each of the sublists in the multilist.

On the concurrent file creation microbenchmark, the performance improvement from dnode_setdirty() is up to 7%. Additionally, the wall clock time spent in spa_sync() is reduced to 15%-40% of the single-threaded case. In terms of cost/reward, once the other bottlenecks are addressed, fixing this bug will provide a medium-large performance gain and require a medium amount of effort to implement.

Sponsored by Intel Corp.

Corresponds to ZoL bug openzfs/zfs#4807

sync_dnodes_task, sda, 0);
/* callback frees sda */
}
taskq_wait(dmu_objset_pool(os)->dp_sync_taskq);

list = &DMU_META_DNODE(os)->dn_dirty_records[txgoff];
while (dr = list_head(list)) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you update it to:
while ((dr = list_head(list)) != NULL)
for remove paretnes gcc warning

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

@ikozhukhov
Copy link

../../common/fs/zfs/dmu_objset.c: In function 'dmu_objset_sync_dnodes':
../../common/fs/zfs/dmu_objset.c:1054:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
while (dn = multilist_sublist_head(list)) {
^
../../common/fs/zfs/dmu_objset.c: In function 'userquota_updates_task':
../../common/fs/zfs/dmu_objset.c:1391:2: error: suggest parentheses around assignment used as truth value [-Werror=parentheses]
while (dn = multilist_sublist_head(list)) {
^
cc1: all warnings being treated as errors

@wangdbang
Copy link

hello, ahrens, is this effective for one zvol under multi-thread writing? thanks.

@ahrens
Copy link
Member Author

ahrens commented Aug 15, 2016

@wangdbang This change parallelizes syncing of different objects (dnodes). Each zvol only has one (modified) object, so there should be no performance change when writing one zvol.

@wangdbang
Copy link

@ahrens Thanks.

@ahrens
Copy link
Member Author

ahrens commented Feb 16, 2017

@zettabot go

1 similar comment
@prakashsurya
Copy link
Member

@zettabot go

@ahrens
Copy link
Member Author

ahrens commented Mar 5, 2017

@zettabot go

@ahrens
Copy link
Member Author

ahrens commented Mar 8, 2017

@zettabot go

@ahrens
Copy link
Member Author

ahrens commented Mar 8, 2017

Also reviewed by @behlendorf, @pzakha, and Brad Lewis

@ahrens
Copy link
Member Author

ahrens commented Mar 8, 2017

@zettabot go

@@ -121,11 +121,14 @@ struct objset {

/* Protected by os_lock */
kmutex_t os_lock;
list_t os_dirty_dnodes[TXG_SIZE];
list_t os_free_dnodes[TXG_SIZE];
struct objset *os_origin_mooch_objset;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't seem to find the meaning of this. Maybe a leftover from earlier revisions?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skiselkov, Thanks for taking a look. Yeah this was due to mismergeing, I thought I fixed it yesterday but turns out it wasn't included in my push. Will fix!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, other than this minor nit, as a whole LGTM.

@ahrens
Copy link
Member Author

ahrens commented Mar 10, 2017

@zettabot go

Copy link

@skiselkov skiselkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ahrens
Copy link
Member Author

ahrens commented Mar 11, 2017

@zettabot go

@ahrens ahrens changed the title multi-threaded spa_sync 7968 multi-threaded spa_sync Mar 14, 2017
@ahrens
Copy link
Member Author

ahrens commented Mar 15, 2017

@zettabot go

Copy link

@danmcd danmcd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

@ahrens ahrens closed this in 4a2a54c Mar 15, 2017
@lundman
Copy link

lundman commented Mar 16, 2017

I thought I did a commit for the missing multilist_destroy in arc.c, but since they are still missing in IllumOS maybe I was dreaming :)

    multilist_destroy(arc_l2c_only->arcs_list[ARC_BUFC_METADATA]);
    multilist_destroy(arc_l2c_only->arcs_list[ARC_BUFC_DATA]);

from openzfsonosx/zfs@ca98a00

@ahrens ahrens deleted the mt-sync branch June 27, 2020 03:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants