fs/fcb: FCB has some async issues, fixing some obvious ones #3382
Conversation
There was a problem hiding this comment.
Hi,
To avoid nested locking I'd suggest to introduce _nolock private variants of functions that are used under mutex similar to how fcb_getnext_nolock() and fcb_getnext() are handled.
and commit message could be a bit more descriptive, eg
fs/fcb: Fix FCB async issues
FCB API can be called from multiple threads. Add missing mutex locking.
|
|
||
| rc = os_mutex_pend(&fcb->f_mtx, OS_WAIT_FOREVER); | ||
| if (rc && rc != OS_NOT_STARTED) { | ||
| return FCB_ERR_ARGS; |
There was a problem hiding this comment.
Would be good to note in header file that negative value means error and not that it is not empty (non-zero value)
(alternative would be to make this int fcb_is_empty(struct fcb *fcb, bool *empty) for more explicit error vs return value
There was a problem hiding this comment.
I think using the return code is good enough, will just update the header to include the updated return code.
| if (rc && rc != OS_NOT_STARTED) { | ||
| return FCB_ERR_ARGS; | ||
| } | ||
|
|
There was a problem hiding this comment.
there is fcb_getnext_nolock() which could be used in loop below to avoid nested locking
|
|
||
| rc = os_mutex_pend(&fcb->f_mtx, OS_WAIT_FOREVER); | ||
| if (rc && rc != OS_NOT_STARTED) { | ||
| return FCB_ERR_ARGS; |
There was a problem hiding this comment.
Would be good to note in header file that negative value means error (or make cnt output parameter similar to suggestion in fcb_is_empty()) (either is fine for me)
There was a problem hiding this comment.
Will update the header file with the description.
| rc = os_mutex_pend(&fcb->f_mtx, OS_WAIT_FOREVER); | ||
| if (rc && rc != OS_NOT_STARTED) { | ||
| return FCB_ERR_ARGS; | ||
| } |
There was a problem hiding this comment.
loop below could use _nolock() variants to avoid nested locking
There was a problem hiding this comment.
I do not think there is any issue with nested locking of a mutex, it uses t_lockcnt from os_task. Maybe more instructions to lock it which is minimal. So, it is either that or more code for nested locking variants, I choose less code.
|
@sjanc I have updated the PR addressing some review comments. Please take a look. |
fs/fcb/src/fcb.c
Outdated
|
|
||
| rc = 0; | ||
| while (!fcb_is_empty(fcb)) { |
There was a problem hiding this comment.
so if there is no fsb_is_empty_nolock() this should handle error checking from fsb_is_empty()
same for fcb_free_sector_cnt() and other that had mutex added if those are used internally
I think overall adding _nolock() would make this simpler
There was a problem hiding this comment.
While I understand what you mean for fcb_is_empty() but for fcb_free_sector_cnt() the fcb_getnext_area() gets called which is already a nolock version. There was no protection for it at all.
There was a problem hiding this comment.
Added fcb_is_empty_nolock()
vrahane
force-pushed
the
vipul/async_fcb_fixes
branch
2 times, most recently
from
e334538 to
1235f52
Compare
vrahane
force-pushed
the
vipul/async_fcb_fixes
branch
2 times, most recently
from
fb48919 to
3e5b0bf
Compare
vrahane
force-pushed
the
vipul/async_fcb_fixes
branch
from
3e5b0bf to
ed67e03
Compare
We are seeing some logging issues related to rotates, on analyzing the FCB code, it seems not protecting FCB access using the mutex might be one of the reasons and causes.