Re: [PATCH] hfsplus: add HFSX subfolder count support

[Sergei Antonov <saproj@gmail.com>]

On Thu, 6 Feb 2014, Vyacheslav Dubeyko wrote:
> 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?

Regarding [1]. 
HFS_FOLDERCOUNT is not a mount option. In hfs_vfsutils.c there is such 
sequence: HFSX signature -> HFS_X flag -> HFS_FOLDERCOUNT flag. No other 
conditions.

Regarding [2].
I analysed this before. A misleading snippet, but it is OK.
This code is a format utility. It has -s parameter indicating "create 
case-sensitive FS". Only HFSX can be case-sensitive and it is the only 
case the utility creates HFSX (it tries to create a more traditional HFS+ 
whenever possible). So in the snippt you show the if condition is an 
awkward way of checking "if we are to create HFSX".

Of course, I've looked through other related places in the code.
 
> > +/*
> > + * 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.

Use macros when functions do the job? No way!
GCC will inline functions without my "inline" hint.

I added checks into them, see the new version of my patch. The code was 
correct without the checks (they only catch cases when FS is already 
corrupted), but I decided to make logic similar to that of Apple.

> > +/*
> > + * 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? 

Conditional decrement - yes. Added in this patch.
Conditional increment - do you mean a check for 'folder_count' from volume 
header (also automatically preventing an integer overflow)? Apple does 
not do it, but we can.
 
> >  	/* 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.

It is not. Trust me.

> >  /* 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.

Expanded it a little in the new patch. Suggest your own text if you still 
think more details are needed. No reason to write much on it. The flag's 
only pecularity is it's HFSX-only, but otherwise it's no more mysterious 
than other flags.

By the way, there is also flag 32 which I hope to add support for 
(encountered fsck errors caused by lack of support of it).

> >  /* 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?

Just fixed = position in both palces. Thanks for telling. I will get 
better at coding style.

-- 
From: Sergei Antonov <saproj@gmail.com>
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, deletion, and rename.

Signed-off-by: Sergei Antonov <saproj@gmail.com>
---
 fs/hfsplus/catalog.c     |   41 +++++++++++++++++++++++++++++++++++++++++
 fs/hfsplus/hfsplus_fs.h  |    1 +
 fs/hfsplus/hfsplus_raw.h |    6 ++++--
 fs/hfsplus/inode.c       |    9 +++++++++
 4 files changed, 55 insertions(+), 2 deletions(-)

diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index 968ce41..32602c6 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,36 @@ int hfsplus_find_cat(struct super_block *sb, u32 cnid,
 	return hfs_brec_find(fd, hfs_find_rec_by_key);
 }
 
+static void hfsplus_subfolders_inc(struct inode *dir)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(dir->i_sb);
+
+	if (test_bit(HFSPLUS_SB_HFSX, &sbi->flags)) {
+		/*
+		 * Increment subfolder count. Note, the value is only meaningful
+		 * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set.
+		 */
+		HFSPLUS_I(dir)->subfolders++;
+	}
+}
+
+static void hfsplus_subfolders_dec(struct inode *dir)
+{
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(dir->i_sb);
+
+	if (test_bit(HFSPLUS_SB_HFSX, &sbi->flags)) {
+		/*
+		 * Decrement subfolder count. Note, the value is only meaningful
+		 * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set.
+		 *
+		 * Check for zero. Some subfolders may have been created
+		 * by an implementation ignorant of this counter.
+		 */
+		if (HFSPLUS_I(dir)->subfolders)
+			HFSPLUS_I(dir)->subfolders--;
+	}
+}
+
 int hfsplus_create_cat(u32 cnid, struct inode *dir,
 		struct qstr *str, struct inode *inode)
 {
@@ -247,6 +279,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);
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
@@ -336,6 +370,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);
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
@@ -380,6 +416,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 +431,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 +444,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) */
 	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..5a12682 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+. */
 } __packed;
 
 /* HFS file info (stolen from hfs.h) */
@@ -301,11 +301,13 @@ struct hfsplus_cat_file {
 	struct hfsplus_fork_raw rsrc_fork;
 } __packed;
 
-/* File attribute bits */
+/* File and folder flag bits */
 #define HFSPLUS_FILE_LOCKED		0x0001
 #define HFSPLUS_FILE_THREAD_EXISTS	0x0002
 #define HFSPLUS_XATTR_EXISTS		0x0004
 #define HFSPLUS_ACL_EXISTS		0x0008
+#define HFSPLUS_HAS_FOLDER_COUNT	0x0010	/* Folder has subfolder count
+						 * (HFSX only) */
 
 /* 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..a4f45bd 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);
+		}
 		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);
+		}
 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
 					 sizeof(struct hfsplus_cat_folder));
 	} else if (HFSPLUS_IS_RSRC(inode)) {
-- 
1.7.10.2