Opened 15 years ago
Closed 15 years ago
#5340 closed bug (fixed)
BLOCK_CACHE_DEBUG_CHANGED is too strict
| Reported by: | romain | Owned by: | axeld |
|---|---|---|---|
| Priority: | normal | Milestone: | R1 |
| Component: | System/Kernel | Version: | R1/alpha1 |
| Keywords: | Cc: | romain.haiku@… | |
| Blocked By: | Blocking: | ||
| Platform: | All |
Description
Ticket #3690 often stops in a block cache check detecting an invalid block buffer change. It seems that this detected error is in fact not an error. Here is what seems to happen:
- The fat filesystem is using the block cache without transactions.
- Block n is acquire through block_cache_get. This initializes the compare pointer in the block cache object, is_dirty is set to false.
- Block n is released through block_cache_put. This adds the block cache object to the unused_block list.
- Block n is acquired through block_get_get_writable. This sets is_dirty to true.
- block_notifier_and_writter wakes up. It will flush the block because it is marked "dirty", not "is_writting", not "busy" and not using transaction. write_cached_block sets "is_dirty" to false.
- Block n is modified in memory.
- Block n is released through block_cache_put: this breaks in put_cached_block because the block is marked as not "dirty" but has a compare pointer that differs from current data.
First I am not sure if this analysis is correct. If it is right this may be solved by marking the block as "is_writing" via a dedicated API, but this field does not seem to be used. So I am not sure what is the purpose of this flag. A second option is to clear the compare data in write_cached_block. This what is done in the provided patch.
In the current situation there is also the problem that we can write the block on disk while it is being modified in memory. I do not know if this has to be handle by the code using the block_cache or in the block_cache itself.
Attachments (1)
Change History (6)
by , 15 years ago
| Attachment: | block_cache_dbg.diff added |
|---|
comment:1 by , 15 years ago
| Component: | - General → System/Kernel |
|---|---|
| Owner: | changed from to |
comment:2 by , 15 years ago
| Cc: | added |
|---|
comment:3 by , 15 years ago
comment:4 by , 15 years ago
| Status: | new → in-progress |
|---|
comment:5 by , 15 years ago
| Resolution: | → fixed |
|---|---|
| Status: | in-progress → closed |
Should be fixed in hrev35390.
Indeed, the current code is broken when used without transactions. However, your fix does not actually solve the problem, it only hides it.
In fact, is_writing has to be correctly used (as much as possible). A quick idea would be to set it to true when a transaction less block is checked out, and set it to false again in put_cached_block() if the used count reaches zero. However, that would lead to a block never being written back if you cache it over the whole mount time (which is something we could simply forbid, though).