DAOS-13938 dfuse: adjust offset in readdir cache entry list #15190
Conversation
The dfuse readdir caching feature stats entries returned by dfs readdir that might be legimitely deleted by another process or another node. When this happens, the dfuse code decides to skip the deleted entry, but might leave the readdir entry cache in an inconsistent state causing the following assertion failure in dfuse_dre_drop(): drc->drc_next_offset == expected_offset + 1 || drc->drc_next_offset == READDIR_EOD This patch fixes the offset in the linked list of cached readdir entries to support skipped entries. Signed-off-by: Johann Lombardi <johann.lombardi@gmail.com>
|
Ticket title is 'dFuse process crashes with the following error sporadically on 2.4 branch' |
| @@ -759,6 +761,12 @@ dfuse_do_readdir(struct dfuse_info *dfuse_info, fuse_req_t req, struct dfuse_obj | |||
| } | |||
|
|
|||
| if (drc) { | |||
| if (oh->doh_rd_nextc != NULL) | |||
There was a problem hiding this comment.
minor nit, any time I have comments, I use braces on if statement and checkpatch never cared. I personally hate the no braces for single line rule style but that is another story.
There was a problem hiding this comment.
I am with you. I will fix once i need to refresh the patch. Probably after @ashleypittman's review.
There was a problem hiding this comment.
I prefer the brevity of no braces for single-line-if statements as they're often simple but I completely agree that if there are comments involved it needs to be clear what code block they belong to.
There was a problem hiding this comment.
This is a complex issue and I need time to come up to speed with the code but it looks exactly the area I'd expect to need updating so I think Johann is on the right track here.
The reproducer for this issue needs two clients, one performing readdir and another modifying the filesystem, the bug relates to a crash in dfuse when performing the readdir request, this will want testing with both ENOENT errors which should be ignored and other errors which should be (probably) propagated to the readdir call.
I'm working through the code and looking at a reproducer and performing a better review of this PR currently.
| @@ -759,6 +761,12 @@ dfuse_do_readdir(struct dfuse_info *dfuse_info, fuse_req_t req, struct dfuse_obj | |||
| } | |||
|
|
|||
| if (drc) { | |||
| if (oh->doh_rd_nextc != NULL) | |||
There was a problem hiding this comment.
I prefer the brevity of no braces for single-line-if statements as they're often simple but I completely agree that if there are comments involved it needs to be clear what code block they belong to.
There was a problem hiding this comment.
This looks correct but I'm not sure it's sufficient to solve the problem entirely.
There are two places that dfs_lookup/create_entry are called in this code and we need to handle both ENOENT and other errors from both sites so at the very least significantly more validation is required here.
The dfuse readdir caching feature stats entries returned by dfs readdir that might be legimitely deleted by another process or another node. When this happens, the dfuse code decides to skip the deleted entry, but might leave the readdir entry cache in an inconsistent state causing the following assertion failure in dfuse_dre_drop():
drc->drc_next_offset == expected_offset + 1 || drc->drc_next_offset == READDIR_EOD
This patch fixes the offset in the linked list of cached readdir entries to support skipped entries.
Before requesting gatekeeper:
Features:(orTest-tag*) commit pragma was used or there is a reason documented that there are no appropriate tags for this PR.Gatekeeper: