The generic/348 test-case has revealed the issue of HFS+ volume corruption after simulated power failure: FSTYP -- hfsplus PLATFORM -- Linux/x86_64 hfsplus-testing-0001 6.15.0-rc4+ #8 SMP PREEMPT_DYNAMIC Thu May 1 16:43:22 PDT 2025 MKFS_OPTIONS -- /dev/loop51 MOUNT_OPTIONS -- /dev/loop51 /mnt/scratch generic/348 _check_generic_filesystem: filesystem on /dev/loop51 is inconsistent (see xfstests-dev/results//generic/348.full for details) The fsck tool complains about Allocation File (block bitmap) corruption as a result of such event. The generic/348 creates a symlink, fsync its parent directory, power fail and mount again the filesystem. Currently, HFS+ logic has several flags HFSPLUS_I_CAT_DIRTY, HFSPLUS_I_EXT_DIRTY, HFSPLUS_I_ATTR_DIRTY, HFSPLUS_I_ALLOC_DIRTY. If inode operation modified the Catalog File, Extents Overflow File, Attributes File, or Allocation File, then inode is marked as dirty and one of the mentioned flags has been set. When hfsplus_file_fsync() has been called, then this set of flags is checked and dirty b-tree or/and block bitmap is flushed. However, block bitmap can be modified during file's content allocation. It means that if we call hfsplus_file_fsync() for directory, then we never flush the modified Allocation File in such case because such inode cannot receive HFSPLUS_I_ALLOC_DIRTY flag. Moreover, this inode-centric model is not good at all because Catalog File, Extents Overflow File, Attributes File, and Allocation File represent the whole state of file system metadata. This inode-centric policy is the main reason of the issue. This patch saves the whole approach of using HFSPLUS_I_CAT_DIRTY, HFSPLUS_I_EXT_DIRTY, HFSPLUS_I_ATTR_DIRTY, and HFSPLUS_I_ALLOC_DIRTY flags. But Catalog File, Extents Overflow File, Attributes File, and Allocation File have associated inodes. And namely these inodes become the mechanism of checking the dirty state of metadata. The hfsplus_file_fsync() method checks the dirtiness of file system metadata by testing HFSPLUS_I_CAT_DIRTY, HFSPLUS_I_EXT_DIRTY, HFSPLUS_I_ATTR_DIRTY, and HFSPLUS_I_ALLOC_DIRTY flags of Catalog File's, Extents Overflow File's, Attributes File's, or Allocation File's inodes. As a result, even if we call hfsplus_file_fsync() for parent folder, then dirty Allocation File will be flushed anyway. Signed-off-by: Viacheslav Dubeyko cc: John Paul Adrian Glaubitz cc: Yangtao Li cc: linux-fsdevel@vger.kernel.org --- fs/hfsplus/attributes.c | 3 +++ fs/hfsplus/catalog.c | 3 +++ fs/hfsplus/dir.c | 6 ++++++ fs/hfsplus/extents.c | 7 +++++++ fs/hfsplus/hfsplus_fs.h | 7 +++++++ fs/hfsplus/inode.c | 27 ++++++++++++++++++++------- fs/hfsplus/super.c | 2 ++ fs/hfsplus/xattr.c | 19 +++++++++++++++++-- 8 files changed, 65 insertions(+), 9 deletions(-) diff --git a/fs/hfsplus/attributes.c b/fs/hfsplus/attributes.c index 4b79cd606276..6585bcea731c 100644 --- a/fs/hfsplus/attributes.c +++ b/fs/hfsplus/attributes.c @@ -241,6 +241,7 @@ int hfsplus_create_attr_nolock(struct inode *inode, const char *name, return err; } + hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ATTR_DIRTY); return 0; @@ -326,6 +327,8 @@ static int __hfsplus_delete_attr(struct inode *inode, u32 cnid, if (err) return err; + hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(inode->i_sb), + HFSPLUS_I_ATTR_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ATTR_DIRTY); return err; } diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c index 02c1eee4a4b8..eef7412a4d58 100644 --- a/fs/hfsplus/catalog.c +++ b/fs/hfsplus/catalog.c @@ -313,6 +313,7 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir, if (S_ISDIR(inode->i_mode)) hfsplus_subfolders_inc(dir); inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir)); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY); hfs_find_exit(&fd); @@ -418,6 +419,7 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, const struct qstr *str) if (type == HFSPLUS_FOLDER) hfsplus_subfolders_dec(dir); inode_set_mtime_to_ts(dir, inode_set_ctime_current(dir)); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY); if (type == HFSPLUS_FILE || type == HFSPLUS_FOLDER) { @@ -540,6 +542,7 @@ int hfsplus_rename_cat(u32 cnid, } err = hfs_brec_insert(&dst_fd, &entry, entry_size); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(dst_dir, HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(src_dir, HFSPLUS_I_CAT_DIRTY); out: diff --git a/fs/hfsplus/dir.c b/fs/hfsplus/dir.c index ca5f74a140ec..0f5eaad738e0 100644 --- a/fs/hfsplus/dir.c +++ b/fs/hfsplus/dir.c @@ -478,6 +478,9 @@ static int hfsplus_symlink(struct mnt_idmap *idmap, struct inode *dir, if (!inode) goto out; + hfs_dbg("dir->i_ino %lu, inode->i_ino %lu\n", + dir->i_ino, inode->i_ino); + res = page_symlink(inode, symname, strlen(symname) + 1); if (res) goto out_err; @@ -526,6 +529,9 @@ static int hfsplus_mknod(struct mnt_idmap *idmap, struct inode *dir, if (!inode) goto out; + hfs_dbg("dir->i_ino %lu, inode->i_ino %lu\n", + dir->i_ino, inode->i_ino); + if (S_ISBLK(mode) || S_ISCHR(mode) || S_ISFIFO(mode) || S_ISSOCK(mode)) init_special_inode(inode, mode, rdev); diff --git a/fs/hfsplus/extents.c b/fs/hfsplus/extents.c index 8e886514d27f..a5f772de9985 100644 --- a/fs/hfsplus/extents.c +++ b/fs/hfsplus/extents.c @@ -121,6 +121,8 @@ static int __hfsplus_ext_write_extent(struct inode *inode, * redirty the inode. Instead the callers have to be careful * to explicily mark the inode dirty, too. */ + set_bit(HFSPLUS_I_EXT_DIRTY, + &HFSPLUS_I(HFSPLUS_EXT_TREE_I(inode->i_sb))->flags); set_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags); return 0; @@ -513,6 +515,8 @@ int hfsplus_file_extend(struct inode *inode, bool zeroout) if (!res) { hip->alloc_blocks += len; mutex_unlock(&hip->extents_lock); + hfsplus_mark_inode_dirty(HFSPLUS_SB(sb)->alloc_file, + HFSPLUS_I_ALLOC_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY); return 0; } @@ -582,6 +586,7 @@ void hfsplus_file_truncate(struct inode *inode) /* XXX: We lack error handling of hfsplus_file_truncate() */ return; } + while (1) { if (alloc_cnt == hip->first_blocks) { mutex_unlock(&fd.tree->tree_lock); @@ -623,5 +628,7 @@ void hfsplus_file_truncate(struct inode *inode) hip->fs_blocks = (inode->i_size + sb->s_blocksize - 1) >> sb->s_blocksize_bits; inode_set_bytes(inode, hip->fs_blocks << sb->s_blocksize_bits); + hfsplus_mark_inode_dirty(HFSPLUS_SB(sb)->alloc_file, + HFSPLUS_I_ALLOC_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_ALLOC_DIRTY); } diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h index 5f891b73a646..122ab57193bb 100644 --- a/fs/hfsplus/hfsplus_fs.h +++ b/fs/hfsplus/hfsplus_fs.h @@ -238,6 +238,13 @@ static inline struct hfsplus_inode_info *HFSPLUS_I(struct inode *inode) return container_of(inode, struct hfsplus_inode_info, vfs_inode); } +#define HFSPLUS_CAT_TREE_I(sb) \ + HFSPLUS_SB(sb)->cat_tree->inode +#define HFSPLUS_EXT_TREE_I(sb) \ + HFSPLUS_SB(sb)->ext_tree->inode +#define HFSPLUS_ATTR_TREE_I(sb) \ + HFSPLUS_SB(sb)->attr_tree->inode + /* * Mark an inode dirty, and also mark the btree in which the * specific type of metadata is stored. diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c index 922ff41df042..cdf08393de44 100644 --- a/fs/hfsplus/inode.c +++ b/fs/hfsplus/inode.c @@ -324,6 +324,7 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end, { struct inode *inode = file->f_mapping->host; struct hfsplus_inode_info *hip = HFSPLUS_I(inode); + struct super_block *sb = inode->i_sb; struct hfsplus_sb_info *sbi = HFSPLUS_SB(inode->i_sb); struct hfsplus_vh *vhdr = sbi->s_vhdr; int error = 0, error2; @@ -344,29 +345,39 @@ int hfsplus_file_fsync(struct file *file, loff_t start, loff_t end, /* * And explicitly write out the btrees. */ - if (test_and_clear_bit(HFSPLUS_I_CAT_DIRTY, &hip->flags)) + if (test_and_clear_bit(HFSPLUS_I_CAT_DIRTY, + &HFSPLUS_I(HFSPLUS_CAT_TREE_I(sb))->flags)) { + clear_bit(HFSPLUS_I_CAT_DIRTY, &hip->flags); error = filemap_write_and_wait(sbi->cat_tree->inode->i_mapping); + } - if (test_and_clear_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags)) { + if (test_and_clear_bit(HFSPLUS_I_EXT_DIRTY, + &HFSPLUS_I(HFSPLUS_EXT_TREE_I(sb))->flags)) { + clear_bit(HFSPLUS_I_EXT_DIRTY, &hip->flags); error2 = filemap_write_and_wait(sbi->ext_tree->inode->i_mapping); if (!error) error = error2; } - if (test_and_clear_bit(HFSPLUS_I_ATTR_DIRTY, &hip->flags)) { - if (sbi->attr_tree) { + if (sbi->attr_tree) { + if (test_and_clear_bit(HFSPLUS_I_ATTR_DIRTY, + &HFSPLUS_I(HFSPLUS_ATTR_TREE_I(sb))->flags)) { + clear_bit(HFSPLUS_I_ATTR_DIRTY, &hip->flags); error2 = filemap_write_and_wait( sbi->attr_tree->inode->i_mapping); if (!error) error = error2; - } else { - pr_err("sync non-existent attributes tree\n"); } + } else { + if (test_and_clear_bit(HFSPLUS_I_ATTR_DIRTY, &hip->flags)) + pr_err("sync non-existent attributes tree\n"); } - if (test_and_clear_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags)) { + if (test_and_clear_bit(HFSPLUS_I_ALLOC_DIRTY, + &HFSPLUS_I(sbi->alloc_file)->flags)) { + clear_bit(HFSPLUS_I_ALLOC_DIRTY, &hip->flags); error2 = filemap_write_and_wait(sbi->alloc_file->i_mapping); if (!error) error = error2; @@ -709,6 +720,8 @@ int hfsplus_cat_write_inode(struct inode *inode) sizeof(struct hfsplus_cat_file)); } + set_bit(HFSPLUS_I_CAT_DIRTY, + &HFSPLUS_I(HFSPLUS_CAT_TREE_I(inode->i_sb))->flags); set_bit(HFSPLUS_I_CAT_DIRTY, &HFSPLUS_I(inode)->flags); out: hfs_find_exit(&fd); diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c index 592d8fbb748c..c963809e0106 100644 --- a/fs/hfsplus/super.c +++ b/fs/hfsplus/super.c @@ -625,6 +625,8 @@ static int hfsplus_fill_super(struct super_block *sb, struct fs_context *fc) } mutex_unlock(&sbi->vh_mutex); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(sb), + HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(sbi->hidden_dir, HFSPLUS_I_CAT_DIRTY); } diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c index 9904944cbd54..31b6cb9db770 100644 --- a/fs/hfsplus/xattr.c +++ b/fs/hfsplus/xattr.c @@ -236,6 +236,7 @@ static int hfsplus_create_attributes_file(struct super_block *sb) put_page(page); } + hfsplus_mark_inode_dirty(HFSPLUS_ATTR_TREE_I(sb), HFSPLUS_I_ATTR_DIRTY); hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY); sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID); @@ -314,8 +315,11 @@ int __hfsplus_setxattr(struct inode *inode, const char *name, hfs_bnode_write(cat_fd.bnode, &entry, cat_fd.entryoffset, sizeof(struct hfsplus_cat_folder)); - hfsplus_mark_inode_dirty(inode, + hfsplus_mark_inode_dirty( + HFSPLUS_CAT_TREE_I(inode->i_sb), HFSPLUS_I_CAT_DIRTY); + hfsplus_mark_inode_dirty(inode, + HFSPLUS_I_CAT_DIRTY); } else { err = -ERANGE; goto end_setxattr; @@ -327,8 +331,11 @@ int __hfsplus_setxattr(struct inode *inode, const char *name, hfs_bnode_write(cat_fd.bnode, &entry, cat_fd.entryoffset, sizeof(struct hfsplus_cat_file)); - hfsplus_mark_inode_dirty(inode, + hfsplus_mark_inode_dirty( + HFSPLUS_CAT_TREE_I(inode->i_sb), HFSPLUS_I_CAT_DIRTY); + hfsplus_mark_inode_dirty(inode, + HFSPLUS_I_CAT_DIRTY); } else { err = -ERANGE; goto end_setxattr; @@ -381,6 +388,8 @@ int __hfsplus_setxattr(struct inode *inode, const char *name, hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset + offsetof(struct hfsplus_cat_folder, flags), cat_entry_flags); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb), + HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY); } else if (cat_entry_type == HFSPLUS_FILE) { cat_entry_flags = hfs_bnode_read_u16(cat_fd.bnode, @@ -392,6 +401,8 @@ int __hfsplus_setxattr(struct inode *inode, const char *name, hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset + offsetof(struct hfsplus_cat_file, flags), cat_entry_flags); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb), + HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY); } else { pr_err("invalid catalog entry type\n"); @@ -862,6 +873,8 @@ static int hfsplus_removexattr(struct inode *inode, const char *name) hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset + offsetof(struct hfsplus_cat_folder, flags), flags); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb), + HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY); } else if (cat_entry_type == HFSPLUS_FILE) { flags = hfs_bnode_read_u16(cat_fd.bnode, cat_fd.entryoffset + @@ -873,6 +886,8 @@ static int hfsplus_removexattr(struct inode *inode, const char *name) hfs_bnode_write_u16(cat_fd.bnode, cat_fd.entryoffset + offsetof(struct hfsplus_cat_file, flags), flags); + hfsplus_mark_inode_dirty(HFSPLUS_CAT_TREE_I(inode->i_sb), + HFSPLUS_I_CAT_DIRTY); hfsplus_mark_inode_dirty(inode, HFSPLUS_I_CAT_DIRTY); } else { pr_err("invalid catalog entry type\n"); -- 2.43.0