Hi Sergei, On Wed, 2014-02-05 at 15:01 +0100, Sergei Antonov wrote: > On Wed, 5 Feb 2014, Vyacheslav Dubeyko wrote: > > > Are you sure that on every HFSX volume folder records in CatalogFile > > have HFSPLUS_HAS_FOLDER_COUNT flag as set? Maybe it can depend from some > > flag or hints in superblock? I simply want to be sure that you tested > > all key situations. > > It only depends on HFSX filesystem. No hints in superblock other than > that. > I worried about correctness of this patch. I've found such details: [1] http://www.opensource.apple.com/source/xnu/xnu-1456.1.26/bsd/hfs/hfs.h /* Macro for incrementing and decrementing the folder count in a cnode * attribute only if the HFS_FOLDERCOUNT bit is set in the mount flags * and kHFSHasFolderCount bit is set in the cnode flags. Currently these * bits are only set for case sensitive HFS+ volumes. */ #define INC_FOLDERCOUNT(hfsmp, cattr) \ if ((hfsmp->hfs_flags & HFS_FOLDERCOUNT) && \ (cattr.ca_recflags & kHFSHasFolderCountMask)) { \ cattr.ca_dircount++; \ } \ #define DEC_FOLDERCOUNT(hfsmp, cattr) \ if ((hfsmp->hfs_flags & HFS_FOLDERCOUNT) && \ (cattr.ca_recflags & kHFSHasFolderCountMask) && \ (cattr.ca_dircount > 0)) { \ cattr.ca_dircount--; \ } [2] diskdev_cmds-557.3.1/newfs_hfs.tproj/makehfs.c /* folder count is only supported on HFSX volumes */ if (dp->flags & kMakeCaseSensitive) { cdp->flags = SWAP_BE16 (kHFSHasFolderCountMask); } So, could you check that there isn't any dependencies from mount options or case sensitivity for HFSX case? Thank you in advance. :) > The new patch follows. With your suggestions included and two > increment/decrement functions to avoid code duplication. > > -- > From: Sergei Antonov <saproj@xxxxxxxxx> > Subject: [PATCH] hfsplus: add HFSX subfolder count support > > This patch adds support for HFSX 'HasFolderCount' flag and a corresponding > 'folderCount' field in folder records. (For reference see HFS_FOLDERCOUNT > and kHFSHasFolderCountBit/kHFSHasFolderCountMask in Apple's source code.) > > Ignoring subfolder count leads to fs errors found by Mac: > ... > Checking catalog hierarchy. > HasFolderCount flag needs to be set (id = 105) > (It should be 0x10 instead of 0) > Incorrect folder count in a directory (id = 2) > (It should be 7 instead of 6) > ... > > Steps to reproduce: > Format with "newfs_hfs -s /dev/diskXXX". > Mount in Linux. > Create a new directory in root. > Unmount. > Run "fsck_hfs /dev/diskXXX". > > The patch handles directory creation, deleteion, and rename. > > Signed-off-by: Sergei Antonov <saproj@xxxxxxxxx> > --- > fs/hfsplus/catalog.c | 29 +++++++++++++++++++++++++++++ > fs/hfsplus/hfsplus_fs.h | 1 + > fs/hfsplus/hfsplus_raw.h | 3 ++- > fs/hfsplus/inode.c | 9 +++++++++ > 4 files changed, 41 insertions(+), 1 deletion(-) > > diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c > index 968ce41..be45bff 100644 > --- a/fs/hfsplus/catalog.c > +++ b/fs/hfsplus/catalog.c > @@ -103,6 +103,8 @@ static int hfsplus_cat_build_record(hfsplus_cat_entry *entry, > folder = &entry->folder; > memset(folder, 0, sizeof(*folder)); > folder->type = cpu_to_be16(HFSPLUS_FOLDER); > + if (test_bit(HFSPLUS_SB_HFSX, &sbi->flags)) > + folder->flags |= cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT); > folder->id = cpu_to_be32(inode->i_ino); > HFSPLUS_I(inode)->create_date = > folder->create_date = > @@ -203,6 +205,24 @@ int hfsplus_find_cat(struct super_block *sb, u32 cnid, > return hfs_brec_find(fd, hfs_find_rec_by_key); > } > > +/* > + * Increment subfolder count. Note, the value is only meaningful > + * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set. > + */ > +static void hfsplus_subfolders_inc(struct inode *dir) > +{ > + HFSPLUS_I(dir)->subfolders++; > +} > + I suppose that using inline keyword or macro declaration can be a better choice. > +/* > + * Decrement subfolder count. Note, the value is only meaningful > + * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set. > + */ > +static void hfsplus_subfolders_dec(struct inode *dir) > +{ > + HFSPLUS_I(dir)->subfolders--; > +} > +HFSPLUS_I(inode) Ditto for decrement case. > int hfsplus_create_cat(u32 cnid, struct inode *dir, > struct qstr *str, struct inode *inode) > { > @@ -247,6 +267,8 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir, > goto err1; > > dir->i_size++; > + if (S_ISDIR(inode->i_mode)) > + hfsplus_subfolders_inc(dir); So, it means for me that we will increment subfolders count unconditionally. For example, we mount HFS+ file system and to make creation of several folders. Then we umount HFS+ and mount again. Finally, creation of new subfolders will be resulted in different values between that we have on volume and calculated in subfolders field. Because we don't store subfolders count ob the volume for the case of HFS+. I suppose that, potentially, it can be confusing and be a source of issues. I think that it makes sense to have conditional increment an decrement operation. Any comment? > dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC; > hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY); > > @@ -336,6 +358,8 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str) > goto out; > > dir->i_size--; > + if (type == HFSPLUS_FOLDER) > + hfsplus_subfolders_dec(dir); Ditto for decrement. > dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC; > hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY); > > @@ -380,6 +404,7 @@ int hfsplus_rename_cat(u32 cnid, > > hfs_bnode_read(src_fd.bnode, &entry, src_fd.entryoffset, > src_fd.entrylength); > + type = be16_to_cpu(entry.type); > > /* create new dir entry with the data from the old entry */ > hfsplus_cat_build_key(sb, dst_fd.search_key, dst_dir->i_ino, dst_name); > @@ -394,6 +419,8 @@ int hfsplus_rename_cat(u32 cnid, > if (err) > goto out; > dst_dir->i_size++; > + if (type == HFSPLUS_FOLDER) > + hfsplus_subfolders_inc(dst_dir); > dst_dir->i_mtime = dst_dir->i_ctime = CURRENT_TIME_SEC; > > /* finally remove the old entry */ > @@ -405,6 +432,8 @@ int hfsplus_rename_cat(u32 cnid, > if (err) > goto out; > src_dir->i_size--; > + if (type == HFSPLUS_FOLDER) > + hfsplus_subfolders_dec(src_dir); > src_dir->i_mtime = src_dir->i_ctime = CURRENT_TIME_SEC; > > /* remove old thread entry */ > diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h > index 08846425b..62d571e 100644 > --- a/fs/hfsplus/hfsplus_fs.h > +++ b/fs/hfsplus/hfsplus_fs.h > @@ -242,6 +242,7 @@ struct hfsplus_inode_info { > */ > sector_t fs_blocks; > u8 userflags; /* BSD user file flags */ > + u32 subfolders; /* Subfolder count (HFSX only) */ "Subfolders count" in comment. It's simply mistyping. > struct list_head open_dir_list; > loff_t phys_size; > > diff --git a/fs/hfsplus/hfsplus_raw.h b/fs/hfsplus/hfsplus_raw.h > index 8ffb3a8..0159137 100644 > --- a/fs/hfsplus/hfsplus_raw.h > +++ b/fs/hfsplus/hfsplus_raw.h > @@ -261,7 +261,7 @@ struct hfsplus_cat_folder { > struct DInfo user_info; > struct DXInfo finder_info; > __be32 text_encoding; > - u32 reserved; > + __be32 subfolders; /* Subfolder count in HFSX. Reserved in HFS+. */ Ditto. > } __packed; > > /* HFS file info (stolen from hfs.h) */ > @@ -306,6 +306,7 @@ struct hfsplus_cat_file { > #define HFSPLUS_FILE_THREAD_EXISTS 0x0002 > #define HFSPLUS_XATTR_EXISTS 0x0004 > #define HFSPLUS_ACL_EXISTS 0x0008 > +#define HFSPLUS_HAS_FOLDER_COUNT 0x0010 /* (HFSX only) */ > I've hoped that you describe flag purpose in more details. :) I mean when it work, in what conditions and so on. > /* HFS+ catalog thread (part of a cat_entry) */ > struct hfsplus_cat_thread { > diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c > index fa929f3..55ffba8 100644 > --- a/fs/hfsplus/inode.c > +++ b/fs/hfsplus/inode.c > @@ -375,6 +375,7 @@ struct inode *hfsplus_new_inode(struct super_block *sb, umode_t mode) > hip->extent_state = 0; > hip->flags = 0; > hip->userflags = 0; > + hip->subfolders = 0; > memset(hip->first_extents, 0, sizeof(hfsplus_extent_rec)); > memset(hip->cached_extents, 0, sizeof(hfsplus_extent_rec)); > hip->alloc_blocks = 0; > @@ -494,6 +495,10 @@ int hfsplus_cat_read_inode(struct inode *inode, struct hfs_find_data *fd) > inode->i_ctime = hfsp_mt2ut(folder->attribute_mod_date); > HFSPLUS_I(inode)->create_date = folder->create_date; > HFSPLUS_I(inode)->fs_blocks = 0; > + if (folder->flags & cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT)) { > + HFSPLUS_I(inode)->subfolders > + = be32_to_cpu(folder->subfolders); Usually, assign symbol is placed on lvalue's line. Maybe it makes sense to declare hip variable for HFSPLUS_I(inode) and place the whole operation on one line? > + } > inode->i_op = &hfsplus_dir_inode_operations; > inode->i_fop = &hfsplus_dir_operations; > } else if (type == HFSPLUS_FILE) { > @@ -566,6 +571,10 @@ int hfsplus_cat_write_inode(struct inode *inode) > folder->content_mod_date = hfsp_ut2mt(inode->i_mtime); > folder->attribute_mod_date = hfsp_ut2mt(inode->i_ctime); > folder->valence = cpu_to_be32(inode->i_size - 2); > + if (folder->flags & cpu_to_be16(HFSPLUS_HAS_FOLDER_COUNT)) { > + folder->subfolders > + = cpu_to_be32(HFSPLUS_I(inode)->subfolders); Ditto. > + } > hfs_bnode_write(fd.bnode, &entry, fd.entryoffset, > sizeof(struct hfsplus_cat_folder)); > } else if (HFSPLUS_IS_RSRC(inode)) { Thanks, Vyacheslav Dubeyko. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
