Opened 10 years ago

Closed 10 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:
Has a Patch: no 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)

block_cache_dbg.diff (557 bytes ) - added by romain 10 years ago.

Download all attachments as: .zip

Change History (6)

by romain, 10 years ago

Attachment: block_cache_dbg.diff added

comment:1 by romain, 10 years ago

Component: - GeneralSystem/Kernel
Owner: changed from nobody to axeld

comment:2 by romain, 10 years ago

Cc: romain.haiku@… added

comment:3 by axeld, 10 years ago

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).

comment:4 by axeld, 10 years ago

Status: newin-progress

comment:5 by axeld, 10 years ago

Resolution: fixed
Status: in-progressclosed

Should be fixed in hrev35390.

Note: See TracTickets for help on using tickets.