Skip to content

fix(fs): align post-write sync with Linux generic_write_sync #2147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
donjuanplatinum wants to merge 3 commits into
base: master
Choose a base branch
from
Open

fix(fs): align post-write sync with Linux generic_write_sync #2147

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f4fc796
fs: align post-write sync with Linux generic_write_sync
donjuanplatinum Jul 19, 2026
854c437
add IOGuard for loopdevice sync
donjuanplatinum Jul 21, 2026
6792521
change maybe_sync_after_write
donjuanplatinum Jul 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion kernel/src/driver/base/block/gendisk/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ use crate::{
driver::{base::device::device_number::DeviceNumber, block::loop_device::LoopDevice},
filesystem::{
devfs::{DevFS, DeviceINode, LockedDevFSInode},
vfs::{utils::DName, IndexNode, InodeMode, Metadata},
vfs::{utils::DName, FilePrivateData, FileType, IndexNode, InodeMode, Metadata},
},
libs::{mutex::MutexGuard, rwlock::RwLock},
};
Expand Down Expand Up @@ -219,6 +219,18 @@ impl IndexNode for GenDisk {
self
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {
file_type == FileType::BlockDevice
}

fn sync_file(
&self,
_datasync: bool,
_data: MutexGuard<FilePrivateData>,
) -> Result<(), SystemError> {
self.sync()
}

fn read_at(
&self,
offset: usize,
Expand Down
12 changes: 12 additions & 0 deletions kernel/src/driver/block/loop_device/loop_device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1010,6 +1010,18 @@ impl IndexNode for LoopDevice {
self
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {
file_type == FileType::BlockDevice
}

fn sync_file(
&self,
_datasync: bool,
_data: MutexGuard<FilePrivateData>,
) -> Result<(), SystemError> {
<Self as BlockDevice>::sync(self)
Comment thread
donjuanplatinum marked this conversation as resolved.

@fslongjin fslongjin Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Retain and fsync the backing File, not only its inode

LOOP_SET_FD currently stores only file.inode(), so after the setup fd is
closed the loop device has lost the backing open-file description and its
FilePrivateData. This is not sufficient for FUSE (file handle and open state),
OverlayFS (the active per-open backing file), credentials, or the file-level
writeback error cursor.

The new sync_file() activates BlockDevice::sync(), which calls inode.sync()
followed by inode.fs().sync_fs(true). That can either miss the actual backing
open file or expand one loop flush into a whole-filesystem sync with unrelated
writeback and duplicate device flushes. Linux loop retains lo_backing_file and
lo_req_flush() calls vfs_fsync() on that File.

Please retain an Arc for the backing object, use its file-level sync
path, and release or replace it only after in-flight I/O is drained. The new
IoGuard is useful for that transition and should remain, but it cannot recover
the open-file context once only an inode was stored. The read/write paths
should use the same retained FilePrivateData as well.

}

fn read_at(
&self,
offset: usize,
Expand Down
17 changes: 16 additions & 1 deletion kernel/src/driver/block/pmem/device.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@ use crate::{
filesystem::{
devfs::{DevFS, DeviceINode, LockedDevFSInode},
kernfs::KernFSInode,
vfs::{utils::DName, FilePrivateData, IndexNode, InodeFlags, InodeId, InodeMode, Metadata},
vfs::{
utils::DName, FilePrivateData, FileType, IndexNode, InodeFlags, InodeId, InodeMode,
Metadata,
},
},
libs::{
align::{page_align_down, page_align_up},
Expand Down Expand Up @@ -222,6 +225,18 @@ impl IndexNode for PmemBlockDevice {
self
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {
file_type == FileType::BlockDevice
}

fn sync_file(
&self,
_datasync: bool,
_data: MutexGuard<FilePrivateData>,
) -> Result<(), SystemError> {
<Self as BlockDevice>::sync(self)
}

fn read_at(
&self,
_offset: usize,
Expand Down
15 changes: 14 additions & 1 deletion kernel/src/driver/block/virtio_blk.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ use crate::{
devfs::{DevFS, DeviceINode, LockedDevFSInode},
kernfs::KernFSInode,
mbr::MbrDiskPartionTable,
vfs::{utils::DName, IndexNode, InodeMode, Metadata},
vfs::{utils::DName, FilePrivateData, FileType, IndexNode, InodeMode, Metadata},
},
init::initcall::INITCALL_POSTCORE,
libs::{
Expand Down Expand Up @@ -849,6 +849,19 @@ impl IndexNode for VirtIOBlkDevice {
fn as_any_ref(&self) -> &dyn core::any::Any {
self
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {
file_type == FileType::BlockDevice
}

fn sync_file(
&self,
_datasync: bool,
_data: MutexGuard<FilePrivateData>,
) -> Result<(), SystemError> {
<Self as BlockDevice>::sync(self)
}

fn read_at(
&self,
_offset: usize,
Expand Down
4 changes: 4 additions & 0 deletions kernel/src/filesystem/ext4/inode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,10 @@ impl IndexNode for LockedExt4Inode {
Some(self.fs())
}

fn supports_post_write_sync(&self, file_type: vfs::FileType) -> bool {
file_type == vfs::FileType::File
}

fn retention_state(&self) -> Option<&InodeRetentionState> {
Some(&self.retention)
}
Expand Down
4 changes: 4 additions & 0 deletions kernel/src/filesystem/fat/fs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1864,6 +1864,10 @@ impl IndexNode for LockedFATInode {
Some(self.fs())
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {

@fslongjin fslongjin Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not opt FAT in before sync_file_range provides a durability boundary

The current FAT sync_file_range() only writes back the selected page-cache
range. It ignores datasync, does not ensure the required FAT chain/directory
entry metadata is committed, and does not issue gendisk.sync() to flush a
volatile device cache. An extending O_SYNC write can therefore return success
while the data or the metadata needed to reach it is still not power-loss
durable.

Linux fat_file_fsync() runs __generic_file_fsync(), synchronizes the FAT
mapping buffers, and finally issues blkdev_issue_flush(). Please first
implement the corresponding file-level range fsync contract, including error
waiting, required metadata, and the final device durability barrier, and only
then opt FAT into generic post-write sync. Adding only a mechanical flush at
the end would not be sufficient if the FAT metadata has not been submitted.

file_type == FileType::File
}

fn mmap(&self, _start: usize, _len: usize, _offset: usize) -> Result<(), SystemError> {
Ok(())
}
Expand Down
4 changes: 4 additions & 0 deletions kernel/src/filesystem/fuse/inode/vfs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,10 @@ impl IndexNode for FuseNode {
self.try_fs()
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {
file_type == FileType::File
}

fn as_any_ref(&self) -> &dyn core::any::Any {
self
}
Expand Down
4 changes: 4 additions & 0 deletions kernel/src/filesystem/overlayfs/inode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,10 @@ impl IndexNode for OvlInode {
Some(self.fs())
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {

@fslongjin fslongjin Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Do not run a second generic sync at the OverlayFS layer

OverlayFS write_at() calls backing_file.pwrite() with the backing File's
O_SYNC/O_DSYNC flags preserved. The backing write therefore already performs
the required post-write synchronization. Returning to the outer File and
opting OvlInode into this helper syncs the same range a second time.

Linux ovl_write_iter() forwards IOCB_SYNC/IOCB_DSYNC to the real file and does
not call generic_write_sync() again after the delegated write. Besides the
extra I/O, the second fsync introduces an error point that Linux does not have:
the delegated write and its sync may succeed, but the outer duplicate sync can
still make the syscall fail.

Please remove this override and let the backing File own the synchronous-write
boundary.

file_type == FileType::File
}

fn open(
&self,
data: crate::libs::mutex::MutexGuard<FilePrivateData>,
Expand Down
4 changes: 4 additions & 0 deletions kernel/src/filesystem/ramfs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,10 @@ impl IndexNode for LockedRamFSInode {
Some(self.fs())
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {
file_type == FileType::File
}

fn mmap(&self, _start: usize, _len: usize, _offset: usize) -> Result<(), SystemError> {
Ok(())
}
Expand Down
4 changes: 4 additions & 0 deletions kernel/src/filesystem/tmpfs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,10 @@ impl IndexNode for LockedTmpfsInode {
Some(self.fs())
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {

@fslongjin fslongjin Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] tmpfs/shmem does not use generic_write_sync()

Linux 6.6 shmem_file_write_iter() returns directly after
generic_perform_write() and does not call generic_write_sync(); its fsync
operation is noop_fsync. Opting tmpfs into this path makes O_SYNC/O_DSYNC
writes perform page-cache range writeback and potentially return errors that
Linux tmpfs does not produce from the write.

Please remove this override. RamFS and tmpfs should not be classified together
just because both are memory-backed: Linux RamFS uses generic_file_write_iter,
while shmem/tmpfs has its own write_iter semantics.

file_type == FileType::File
}

@fslongjin fslongjin Jul 23, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we should add a default impl or make a better design instead of write these duplicated code.


fn mmap(&self, _start: usize, _len: usize, _offset: usize) -> Result<(), SystemError> {
Ok(())
}
Expand Down
59 changes: 33 additions & 26 deletions kernel/src/filesystem/vfs/file.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,10 +205,6 @@ struct WriteConfig {
update_offset: bool,
/// 偏移量更新方式
offset_update: OffsetUpdate,
/// 文件标志
flags: FileFlags,
/// inode 标志
inode_flags: InodeFlags,
}
/// Namespace fd backing data, typically created from /proc/thread-self/ns/* files.
#[derive(Clone)]
Expand Down Expand Up @@ -828,9 +824,16 @@ impl File {

fn maybe_sync_after_write(
&self,
file_type: FileType,
start: usize,
written_len: usize,
flags: FileFlags,
inode_flags: InodeFlags,
) -> Result<(), SystemError> {
if written_len == 0 || !self.inode.supports_post_write_sync(file_type) {

@fslongjin fslongjin Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Include superblock synchronous semantics before this early return

The later inode_sync value only checks Metadata.flags::S_SYNC.
MountFSInode::metadata() does not fold MountFlags::SYNCHRONOUS into that field,
so a normal write on a mount -o sync filesystem still skips post-write sync.
Linux iocb_is_dsync() uses IS_SYNC(inode), which includes both inode S_SYNC and
SB_SYNCHRONOUS.

Please include the mount/superblock synchronous state in the trigger. Also,
test the cheap sync flags first and only query the filesystem policy when a
positive-length write actually requires synchronization. In the current
order, every ordinary asynchronous write performs this virtual call, and a
MountFSInode performs another delegated virtual call, even though no sync is
needed.

return Ok(());
}

// O_SYNC 包含 O_DSYNC 位，所以只需检查 O_DSYNC 即可判断是否需要数据同步
let need_data_sync = flags.contains(FileFlags::O_DSYNC);
// 检查是否需要元数据同步（O_SYNC = __O_SYNC | O_DSYNC）
Expand All @@ -840,12 +843,15 @@ impl File {
let inode_sync = inode_flags.contains(InodeFlags::S_SYNC);

if need_data_sync || inode_sync {
if need_metadata_sync || inode_sync {
// O_SYNC 或 S_SYNC: 完整同步（数据 + 元数据）
self.inode.sync_file(false, self.private_data.lock())?;
let end = start.saturating_add(written_len).saturating_sub(1);
if need_metadata_sync {
// O_SYNC: 完整同步（数据 + 元数据）
self.inode

@fslongjin fslongjin Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Preserve the open-file writeback error contract here

Calling inode.sync_file_range() directly bypasses the wb_error_seq check that
File::sync_range_and_check_wb_error() already performs. If asynchronous
writeback recorded EIO or ENOSPC after this File was opened, and this specific
range writeback now succeeds, an O_SYNC/O_DSYNC write can incorrectly return
the byte count without reporting or advancing that error for this open file
description.

Linux generic_write_sync() reaches vfs_fsync_range(), and
file_write_and_wait_range() always calls file_check_and_advance_wb_err() after
the range writeback. Please route this through the existing File helper, for
example:

let datasync = !need_metadata_sync;
self.sync_range_and_check_wb_error(start, end, datasync)?;

Advancing the file offset before a later sync error is already consistent with
Linux and should not be rolled back.

.sync_file_range(start, end, false, self.private_data.lock())?;
} else {
// O_DSYNC: 仅数据同步
self.inode.sync_file(true, self.private_data.lock())?;
// O_DSYNC 或 S_SYNC: datasync；完整同步只由 O_SYNC 决定。
self.inode
.sync_file_range(start, end, true, self.private_data.lock())?;
}
}
Ok(())
Expand Down Expand Up @@ -919,7 +925,6 @@ impl File {
}
}

self.maybe_sync_after_write(config.flags, config.inode_flags)?;
Ok(written_len)
}
/// @brief 创建一个新的文件对象
Expand Down Expand Up @@ -1484,31 +1489,32 @@ impl File {
WriteConfig {
update_offset,
offset_update: OffsetUpdate::StoreEnd,
flags,
inode_flags,
},
)
.map(|written_len| (actual_offset, written_len))
};
return match self.append_lock_domain.as_ref() {
let (actual_offset, written_len) = match self.append_lock_domain.as_ref() {
Some(domain) => with_inode_append_lock(domain.key(&md), append_write),
None => append_write(),
};
}?;
self.maybe_sync_after_write(file_type, actual_offset, written_len, flags, inode_flags)?;
return Ok(written_len);
}

let actual_offset = offset;
let actual_len = self.limit_write_len_by_fsize(file_type, actual_offset, len)?;

self.write_at_and_finalize(
let written_len = self.write_at_and_finalize(
actual_offset,
actual_len,
buf,
WriteConfig {
update_offset,
offset_update: OffsetUpdate::Add,
flags,
inode_flags,
},
)
)?;
self.maybe_sync_after_write(file_type, actual_offset, written_len, flags, inode_flags)?;
Ok(written_len)
}

pub fn do_write_user(
Expand Down Expand Up @@ -1553,31 +1559,32 @@ impl File {
WriteConfig {
update_offset,
offset_update: OffsetUpdate::StoreEnd,
flags,
inode_flags,
},
)
.map(|written_len| (actual_offset, written_len))
};
return match self.append_lock_domain.as_ref() {
let (actual_offset, written_len) = match self.append_lock_domain.as_ref() {
Some(domain) => with_inode_append_lock(domain.key(&md), append_write),
None => append_write(),
};
}?;
self.maybe_sync_after_write(file_type, actual_offset, written_len, flags, inode_flags)?;
return Ok(written_len);
}

let actual_offset = offset;
let actual_len = self.limit_write_len_by_fsize(file_type, actual_offset, len)?;

self.write_user_at_and_finalize(
let written_len = self.write_user_at_and_finalize(
actual_offset,
actual_len,
reader,
WriteConfig {
update_offset,
offset_update: OffsetUpdate::Add,
flags,
inode_flags,
},
)
)?;
self.maybe_sync_after_write(file_type, actual_offset, written_len, flags, inode_flags)?;
Ok(written_len)
}

/// Write to the file from a userspace buffer.
Expand Down
9 changes: 9 additions & 0 deletions kernel/src/filesystem/vfs/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -587,6 +587,15 @@ pub trait IndexNode: Any + Sync + Send + Debug + CastFromSync {
/// protocol flags.
fn adjust_file_mode_after_open(&self, _data: &FilePrivateData, _mode: &mut FileMode) {}

/// Whether the VFS write path should apply Linux `generic_write_sync()`
/// semantics after a successful positive-length write.
///
/// This is intentionally opt-in: many pseudo files are reported as regular
/// files but their Linux write paths do not perform generic write syncing.
fn supports_post_write_sync(&self, _file_type: FileType) -> bool {

@fslongjin fslongjin Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[P1] Model this as a write-operation policy, not a default-false inode capability

Linux does not decide whether to run generic_write_sync() from an inode
property. The concrete file_operations.write_iter either invokes the helper,
forwards IOCB_SYNC/IOCB_DSYNC to another File, or implements unrelated write
semantics.

Making this a default-false IndexNode method creates a silent durability
failure mode: every new disk filesystem and every transparent wrapper must
remember to opt in or forward this method, otherwise O_SYNC/O_DSYNC writes
still return success without syncing. The OverlayFS and tmpfs overrides in
this patch already show that the boolean is also too weak to distinguish
"generic sync", "handled by the delegated write", and "no generic sync".

Please make the selected open-file/write operation own this decision. A
bounded intermediate design would cache an explicit per-open policy such as
Generic / HandledByWrite / None in File. That avoids a full VFS rewrite while
making the responsibility explicit and removing the per-write wrapper
dispatch.

false
}

/// @brief 关闭文件
///
/// @return 成功：Ok()
Expand Down
4 changes: 4 additions & 0 deletions kernel/src/filesystem/vfs/mount/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3611,6 +3611,10 @@ impl IndexNode for MountFSInode {
self.dentry.inode.adjust_file_mode_after_open(data, mode)
}

fn supports_post_write_sync(&self, file_type: FileType) -> bool {
self.dentry.inode.supports_post_write_sync(file_type)
}

fn mmap(&self, start: usize, len: usize, offset: usize) -> Result<(), SystemError> {
return self.dentry.inode.mmap(start, len, offset);
}
Expand Down
Loading