DAOS-19386 dfuse: move notify calls to an asynchronous queue - #18806
DAOS-19386 dfuse: move notify calls to an asynchronous queue#18806mjmac wants to merge 2 commits into
Conversation
|
Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18806/1/testReport/ |
e77e690 to
3b53090
Compare
|
Test stage Functional on EL 9 completed with status FAILURE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net//job/daos-stack/job/daos/view/change-requests/job/PR-18806/1/execution/node/1267/log |
mjmac
changed the title
|
Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18806/2/testReport/ |
|
Ticket title is 'NLT stage timing out (~63 min) on master and PR builds — daos container create fault injection appears to hang' |
|
Test stage NLT completed with status UNSTABLE. https://jenkins-3.daos.hpc.amslabs.hpecorp.net/job/daos-stack/job/daos//view/change-requests/job/PR-18806/3/testReport/ |
Don't allow a stuck child process to tank the entire NLT
run. When a stall is detected, attempt to capture stack
traces and fail early.
The /proc dump shows a real stall directly. From the hang that
motivated this patch: the stalled child holds the parent-dir lock
while waiting on its LOOKUP reply, and every dfuse worker is blocked
on that same lock trying to write a dentry invalidation:
TID 34163: comm=daos wchan=request_wait_answer state=S
[<0>] request_wait_answer+0xfa/0x210 [fuse]
[<0>] fuse_simple_request+0x1b8/0x330 [fuse]
[<0>] fuse_lookup_name+0xa4/0x1c0 [fuse]
[<0>] fuse_lookup+0x66/0x190 [fuse]
[<0>] __lookup_hash+0x70/0xa0
[<0>] __filename_create+0x87/0x150
[<0>] do_mkdirat+0x4c/0x160
[<0>] __x64_sys_mkdir+0x47/0x70
TID 29212: comm=dfuse worker wchan=fuse_reverse_inval_entry state=D
[<0>] fuse_reverse_inval_entry+0x40/0x210 [fuse]
[<0>] fuse_notify+0x287/0x500 [fuse]
[<0>] fuse_dev_do_write+0x305/0x4e0 [fuse]
[<0>] fuse_dev_write+0x50/0x80 [fuse]
TID 29213, 29214: identical (the whole worker pool)
Signed-off-by: Michael MacDonald <github@macdonald.cx>
Most fuse_lowlevel_notify_* calls were already effectively fire-and-forget, but blocked the calling worker while the kernel processed them. In certain scenarios, e.g. mkdir racing with multiple setxattrs in the same directory, this pattern could result in all threads being blocked, and then the mount would hang. Making the notify calls properly asynchronous means that workers will not block on waiting for a response. A dedicated thread now handles queued notifications, with some logic to coalesce notifications. duns_create_path, which does need the invalidation's effect before returning, now polls until the path is confirmed bound to the new container before returning success. Signed-off-by: Michael MacDonald <github@macdonald.cx>
d3b0136 to
e3242d1
Compare
| il_reply.fir_flags |= DFUSE_IOCTL_FLAGS_MCACHE; | ||
|
|
||
| if (oh->doh_writeable) { | ||
| rc = fuse_lowlevel_notify_inval_inode(dfuse_info->di_session, |
There was a problem hiding this comment.
from your original analysis, inval_inode() and notify_delete() in unlink, did not report the issue. so on my side i had left them sync. did the AI analysis also show an issue there?
These block on writeback (not i_rwsem), so they were never part of the reported notify_inval_entry deadlock.
There was a problem hiding this comment.
@mchaarawi: This change came from my preference to provide a coherent approach to the notify calls. It's how I would have approached a similarly-shaped problem on the control plane side. Instead of spreading responsibility around for being careful about how these calls work, just put it all in one place and make it obvious that it's the right way to do it. But, it is a wider change than just fixing the immediate deadlock problem.
| struct stat finfo; | ||
| /* | ||
| * This next stat will cause dfuse to lookup the entry point and perform a | ||
| * container connect, therefore this data will be read from root of the new | ||
| * container, not the directory. | ||
| * | ||
| * TODO: This could call getxattr to verify success. | ||
| /* Confirm dfuse has looked up the entry point and connected to the new | ||
| * container. | ||
| */ | ||
| rc = stat(path, &finfo); | ||
| if (rc) { | ||
| rc = errno; | ||
| D_ERROR("Failed to access new container: %d (%s)\n", rc, strerror(rc)); | ||
| goto err_link; | ||
| } | ||
| rc = duns_wait_for_resolution(path, attrp->da_cuuid); | ||
| if (rc) | ||
| goto err_verify; |
There was a problem hiding this comment.
i understand now why you are doing this now.
technically this stat was there (as the comment says) to force the lookup. but with the new async invalidation, there is a chance it might hit a stale dentry. maybe for now we don't even do anything here.
but i guess your test exposes something inthis case.
your PR actually exposes this issue more bec you are also moving the inval_inode and notify_delete() as async, so maybe you do hit this issue in your test. but normally maybe o
3 participants
Most fuse_lowlevel_notify_* calls were already effectively
fire-and-forget, but blocked the calling worker while the kernel
processed them. In certain scenarios, e.g. mkdir racing with
multiple setxattrs in the same directory, this pattern could
result in all threads being blocked, and then the mount would hang.
Making the notify calls properly asynchronous means that workers
will not block on waiting for a response. A dedicated thread now
handles queued notifications, with some logic to coalesce notifications.
duns_create_path, which does need the invalidation's effect before
returning, now polls until the path is confirmed bound to the
new container before returning success.
Signed-off-by: Michael MacDonald github@macdonald.cx