Skip to content

Commit a7b70c6

Browse files
committed
Kernel: Fix broken disk cache behavior with O_DIRECT
When writing into a block via an O_DIRECT open file description, we would first flush every dirty block *except* the one we're about to write into. The purpose of flushing is to ensure coherency when mixing direct and indirect accesses to the same file. This patch fixes the issue by only flushing the affected block. Our behavior was totally backwards and could end up doing a lot of unnecessary work while avoiding the work that actually mattered.
1 parent 95df924 commit a7b70c6

File tree

1 file changed

+21
-18
lines changed

1 file changed

+21
-18
lines changed

Kernel/FileSystem/BlockBasedFileSystem.cpp

Lines changed: 21 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ class DiskCache {
3535
~DiskCache() = default;
3636

3737
bool is_dirty() const { return !m_dirty_list.is_empty(); }
38+
bool entry_is_dirty(CacheEntry const& entry) const { return m_dirty_list.contains(entry); }
3839

3940
void mark_all_clean()
4041
{
@@ -52,13 +53,20 @@ class DiskCache {
5253
m_clean_list.prepend(entry);
5354
}
5455

56+
CacheEntry* get(BlockBasedFileSystem::BlockIndex block_index) const
57+
{
58+
auto it = m_hash.find(block_index);
59+
if (it == m_hash.end())
60+
return nullptr;
61+
auto& entry = const_cast<CacheEntry&>(*it->value);
62+
VERIFY(entry.block_index == block_index);
63+
return &entry;
64+
}
65+
5566
CacheEntry& ensure(BlockBasedFileSystem::BlockIndex block_index) const
5667
{
57-
if (auto it = m_hash.find(block_index); it != m_hash.end()) {
58-
auto& entry = const_cast<CacheEntry&>(*it->value);
59-
VERIFY(entry.block_index == block_index);
60-
return entry;
61-
}
68+
if (auto* entry = get(block_index))
69+
return *entry;
6270

6371
if (m_clean_list.is_empty()) {
6472
// Not a single clean entry! Flush writes and try again.
@@ -261,19 +269,14 @@ void BlockBasedFileSystem::flush_specific_block_if_needed(BlockIndex index)
261269
m_cache.with_exclusive([&](auto& cache) {
262270
if (!cache->is_dirty())
263271
return;
264-
Vector<CacheEntry*, 32> cleaned_entries;
265-
cache->for_each_dirty_entry([&](CacheEntry& entry) {
266-
if (entry.block_index != index) {
267-
size_t base_offset = entry.block_index.value() * block_size();
268-
auto entry_data_buffer = UserOrKernelBuffer::for_kernel_buffer(entry.data);
269-
[[maybe_unused]] auto rc = file_description().write(base_offset, entry_data_buffer, block_size());
270-
cleaned_entries.append(&entry);
271-
}
272-
});
273-
// NOTE: We make a separate pass to mark entries clean since marking them clean
274-
// moves them out of the dirty list which would disturb the iteration above.
275-
for (auto* entry : cleaned_entries)
276-
cache->mark_clean(*entry);
272+
auto* entry = cache->get(index);
273+
if (!entry)
274+
return;
275+
if (!cache->entry_is_dirty(*entry))
276+
return;
277+
size_t base_offset = entry->block_index.value() * block_size();
278+
auto entry_data_buffer = UserOrKernelBuffer::for_kernel_buffer(entry->data);
279+
(void)file_description().write(base_offset, entry_data_buffer, block_size());
277280
});
278281
}
279282

0 commit comments

Comments
 (0)