[PATCH] hfsplus: add HFSX subfolder count support

[PATCH] hfsplus: add HFSX subfolder count support

[Sergei Antonov]

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".

Signed-off-by: Sergei Antonov <saproj@gmail.com>
---
 fs/hfsplus/catalog.c     |    7 +++++++
 fs/hfsplus/hfsplus_fs.h  |    1 +
 fs/hfsplus/hfsplus_raw.h |    3 ++-
 fs/hfsplus/inode.c       |    3 +++
 4 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index 968ce41..bc770303 100644
--- a/fs/hfsplus/catalog.c
+++ b/fs/hfsplus/catalog.c
@@ -103,12 +103,15 @@ 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 =
 			folder->content_mod_date =
 			folder->attribute_mod_date =
 			folder->access_date = hfsp_now2mt();
+		HFSPLUS_I(inode)->folder_count = 0;
 		hfsplus_cat_set_perms(inode, &folder->permissions);
 		if (inode == sbi->hidden_dir)
 			/* invisible and namelocked */
@@ -247,6 +250,8 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
 		goto err1;
 
 	dir->i_size++;
+	if (S_ISDIR(inode->i_mode))
+		HFSPLUS_I(dir)->folder_count++;
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
@@ -336,6 +341,8 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
 		goto out;
 
 	dir->i_size--;
+	if (type == HFSPLUS_FOLDER)
+		HFSPLUS_I(dir)->folder_count--;
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 08846425b..2c22cd2 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 folder_count;	/* subfolder count if HFSPLUS_HAS_FOLDER_COUNT is set */
 	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..6529629 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 folder_count;	/* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT is set. Reserved otherwise. */
 } __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
 
 /* 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..7d18221 100644
--- a/fs/hfsplus/inode.c
+++ b/fs/hfsplus/inode.c
@@ -494,6 +494,7 @@ 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;
+		HFSPLUS_I(inode)->folder_count = be32_to_cpu(folder->folder_count);
 		inode->i_op = &hfsplus_dir_inode_operations;
 		inode->i_fop = &hfsplus_dir_operations;
 	} else if (type == HFSPLUS_FILE) {
@@ -566,6 +567,8 @@ 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->folder_count = cpu_to_be32(HFSPLUS_I(inode)->folder_count);
 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
 					 sizeof(struct hfsplus_cat_folder));
 	} else if (HFSPLUS_IS_RSRC(inode)) {
-- 
1.7.10.2

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

[Vyacheslav Dubeyko]

Hi Sergey,

On Feb 3, 2014, at 9:47 PM, Sergei Antonov wrote:

> 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".
> 
> Signed-off-by: Sergei Antonov <saproj@gmail.com>

Thank you for the patch. Please, find my remarks below.
I am going to review your patch more deeper tomorrow.

> ---
> fs/hfsplus/catalog.c     |    7 +++++++
> fs/hfsplus/hfsplus_fs.h  |    1 +
> fs/hfsplus/hfsplus_raw.h |    3 ++-
> fs/hfsplus/inode.c       |    3 +++
> 4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 968ce41..bc770303 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -103,12 +103,15 @@ 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 =
> 			folder->content_mod_date =
> 			folder->attribute_mod_date =
> 			folder->access_date = hfsp_now2mt();
> +		HFSPLUS_I(inode)->folder_count = 0;
> 		hfsplus_cat_set_perms(inode, &folder->permissions);
> 		if (inode == sbi->hidden_dir)
> 			/* invisible and namelocked */
> @@ -247,6 +250,8 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
> 		goto err1;
> 
> 	dir->i_size++;
> +	if (S_ISDIR(inode->i_mode))
> +		HFSPLUS_I(dir)->folder_count++;
> 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
> 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
> 
> @@ -336,6 +341,8 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
> 		goto out;
> 
> 	dir->i_size--;
> +	if (type == HFSPLUS_FOLDER)
> +		HFSPLUS_I(dir)->folder_count--;
> 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
> 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
> 
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 08846425b..2c22cd2 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 folder_count;	/* subfolder count if HFSPLUS_HAS_FOLDER_COUNT is set */

I am afraid this line breaks kernel code style. I mean that
comment is too long for one line and it hasn't white space
between code and comment.

> 	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..6529629 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 folder_count;	/* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT is set. Reserved otherwise. */

Ditto. Too long comment for one line.

> } __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
> 
> /* 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..7d18221 100644
> --- a/fs/hfsplus/inode.c
> +++ b/fs/hfsplus/inode.c
> @@ -494,6 +494,7 @@ 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;
> +		HFSPLUS_I(inode)->folder_count = be32_to_cpu(folder->folder_count);

You check HFSPLUS_HAS_FOLDER_COUNT flag below in hfsplus_cat_write_inode()
but you don't check here. Are you sure that it is right way?

Thanks,
Vyacheslav Dubeyko.

> 		inode->i_op = &hfsplus_dir_inode_operations;
> 		inode->i_fop = &hfsplus_dir_operations;
> 	} else if (type == HFSPLUS_FILE) {
> @@ -566,6 +567,8 @@ 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->folder_count = cpu_to_be32(HFSPLUS_I(inode)->folder_count);
> 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
> 					 sizeof(struct hfsplus_cat_folder));
> 	} else if (HFSPLUS_IS_RSRC(inode)) {
> -- 
> 1.7.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sergei Antonov]

> Vyacheslav Dubeyko wrote:
>> diff --git a/fs/hfsplus/inode.c b/fs/hfsplus/inode.c
>> index fa929f3..7d18221 100644
>> --- a/fs/hfsplus/inode.c
>> +++ b/fs/hfsplus/inode.c
>> @@ -494,6 +494,7 @@ 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;
>> +        HFSPLUS_I(inode)->folder_count = be32_to_cpu(folder->folder_count);
> 
> You check HFSPLUS_HAS_FOLDER_COUNT flag below in hfsplus_cat_write_inode()
> but you don't check here. Are you sure that it is right way?

Thanks for review, Vyacheslav.
Yes. I omit a check here and in decrement/increment code. The only check which must be done is when the record is written to the disk.

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

[Vyacheslav Dubeyko]

Hi Sergei,

On Tue, 2014-02-04 at 01:25 +0100, Sergei Antonov wrote:

I have made some additional comments. Please, see below.

On Mon, 2014-02-03 at 19:47 +0100, Sergei Antonov wrote:

[snip]
> 
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
>  fs/hfsplus/catalog.c     |    7 +++++++
>  fs/hfsplus/hfsplus_fs.h  |    1 +
>  fs/hfsplus/hfsplus_raw.h |    3 ++-
>  fs/hfsplus/inode.c       |    3 +++
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
> index 968ce41..bc770303 100644
> --- a/fs/hfsplus/catalog.c
> +++ b/fs/hfsplus/catalog.c
> @@ -103,12 +103,15 @@ 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);

I suppose that it will be better to use OR operation during flag set.
What do you think?

>  		folder->id = cpu_to_be32(inode->i_ino);
>  		HFSPLUS_I(inode)->create_date =
>  			folder->create_date =
>  			folder->content_mod_date =
>  			folder->attribute_mod_date =
>  			folder->access_date = hfsp_now2mt();
> +		HFSPLUS_I(inode)->folder_count = 0;

Is it zeroing really necessary? The memset operation doesn't set it earlier?

>  		hfsplus_cat_set_perms(inode, &folder->permissions);
>  		if (inode == sbi->hidden_dir)
>  			/* invisible and namelocked */
> @@ -247,6 +250,8 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
>  		goto err1;
>  
>  	dir->i_size++;
> +	if (S_ISDIR(inode->i_mode))
> +		HFSPLUS_I(dir)->folder_count++;

Yes, it needs check of flag here.

>  	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
>  	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
>  
> @@ -336,6 +341,8 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
>  		goto out;
>  
>  	dir->i_size--;
> +	if (type == HFSPLUS_FOLDER)
> +		HFSPLUS_I(dir)->folder_count--;

Ditto. It needs to check flag here.

>  	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
>  	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
>  
> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
> index 08846425b..2c22cd2 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 folder_count;	/* subfolder count if HFSPLUS_HAS_FOLDER_COUNT is set */

Why do you place variable here? What consideration have you?

>  	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..6529629 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 folder_count;	/* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT is set. Reserved otherwise. */
>  } __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

I am no fully confident that this constant is correct.
Where do you find it? All previous constant is file related, as far as I
can see. Why do you place folder related declaration in this group of
constants?

Thanks,
Vyacheslav Dubeyko.

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

[Sergei Antonov]

[-- Attachment #1: Type: text/plain, Size: 5593 bytes --]

On 4 February 2014 08:10, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:
> Hi Sergei,
>
> On Tue, 2014-02-04 at 01:25 +0100, Sergei Antonov wrote:
>
> I have made some additional comments. Please, see below.
>
> On Mon, 2014-02-03 at 19:47 +0100, Sergei Antonov wrote:
>
> [snip]
>>
>> Signed-off-by: Sergei Antonov <saproj@gmail.com>
>> ---
>>  fs/hfsplus/catalog.c     |    7 +++++++
>>  fs/hfsplus/hfsplus_fs.h  |    1 +
>>  fs/hfsplus/hfsplus_raw.h |    3 ++-
>>  fs/hfsplus/inode.c       |    3 +++
>>  4 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
>> index 968ce41..bc770303 100644
>> --- a/fs/hfsplus/catalog.c
>> +++ b/fs/hfsplus/catalog.c
>> @@ -103,12 +103,15 @@ 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);
>
> I suppose that it will be better to use OR operation during flag set.
> What do you think?

I do not mind. See the reworked patch in attachment.

>>               folder->id = cpu_to_be32(inode->i_ino);
>>               HFSPLUS_I(inode)->create_date =
>>                       folder->create_date =
>>                       folder->content_mod_date =
>>                       folder->attribute_mod_date =
>>                       folder->access_date = hfsp_now2mt();
>> +             HFSPLUS_I(inode)->folder_count = 0;
>
> Is it zeroing really necessary?

Yes.

> The memset operation doesn't set it earlier?

No. Where is memset for 'hfsplus_inode_info'? Without initialization
this field was garbage.

In the reworked patch the initialization is still done, although I
moved it to hfsplus_new_inode().

>>               hfsplus_cat_set_perms(inode, &folder->permissions);
>>               if (inode == sbi->hidden_dir)
>>                       /* invisible and namelocked */
>> @@ -247,6 +250,8 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
>>               goto err1;
>>
>>       dir->i_size++;
>> +     if (S_ISDIR(inode->i_mode))
>> +             HFSPLUS_I(dir)->folder_count++;
>
> Yes, it needs check of flag here.
>
>>       dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
>>       hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
>>
>> @@ -336,6 +341,8 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
>>               goto out;
>>
>>       dir->i_size--;
>> +     if (type == HFSPLUS_FOLDER)
>> +             HFSPLUS_I(dir)->folder_count--;
>
> Ditto. It needs to check flag here.

To do a check I guess I have to read flags like this:
   off = fd.entryoffset + offsetof(struct hfsplus_cat_folder, flags);
   flags = hfs_bnode_read_u16(fd.bnode, off);
This is a performance penalty for an _unnecessary_ check.
In the reworked patch I wrote a comment telling the value is not
always meaningful.

>>       dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
>>       hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
>>
>> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
>> index 08846425b..2c22cd2 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 folder_count;       /* subfolder count if HFSPLUS_HAS_FOLDER_COUNT is set */
>
> Why do you place variable here? What consideration have you?

Two obvious considerations.
1st it is related to 'userflags' because it is also a copy of a
catalog record field.
2nd it is protected by 'i_mutex' in hfsplus_create_cat,
hfsplus_delete_cat, hfsplus_rename_cat.

>>       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..6529629 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 folder_count;    /* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT is set. Reserved otherwise. */
>>  } __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
>
> I am no fully confident that this constant is correct.
> Where do you find it?

As I wrote in my original message: see HFS_FOLDERCOUNT and
kHFSHasFolderCountBit/kHFSHasFolderCountMask in Apple's source code.
The value is from there, the name is by me.
To become confident the constant is correct one has to read that
(publicly available) source code and/or look at the test-case with and
without the patch.

> All previous constant is file related, as far as I
> can see. Why do you place folder related declaration in this group of
> constants?

It is in the same group with other constants. They are all flags in
file or folder records. They follow 1,2,4,8,16 sequence although
1,2,4,8 are only for files and 16 is only for folders.

The reworked patch is attached. Take a look, please.
In addition to changes already mentioned, it adds proper handling of
directory rename. I overlooked it initially.

[-- Attachment #2: 0001-hfsplus-add-HFSX-subfolder-count-support.patch.txt --]
[-- Type: text/plain, Size: 6179 bytes --]

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".

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

diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
index 968ce41..04228d6 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 =
@@ -247,6 +249,14 @@ int hfsplus_create_cat(u32 cnid, struct inode *dir,
 		goto err1;
 
 	dir->i_size++;
+
+	/*
+	 * Increment folder count. Note, the value is only meaningful
+	 * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set.
+	 */
+	if (S_ISDIR(inode->i_mode))
+		HFSPLUS_I(dir)->folder_count++;
+
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
@@ -336,6 +346,14 @@ int hfsplus_delete_cat(u32 cnid, struct inode *dir, struct qstr *str)
 		goto out;
 
 	dir->i_size--;
+
+	/*
+	 * Decrement folder count. Note, the value is only meaningful
+	 * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set.
+	 */
+	if (type == HFSPLUS_FOLDER)
+		HFSPLUS_I(dir)->folder_count--;
+
 	dir->i_mtime = dir->i_ctime = CURRENT_TIME_SEC;
 	hfsplus_mark_inode_dirty(dir, HFSPLUS_I_CAT_DIRTY);
 
@@ -380,6 +398,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 +413,14 @@ int hfsplus_rename_cat(u32 cnid,
 	if (err)
 		goto out;
 	dst_dir->i_size++;
+
+	/*
+	 * Increment folder count. Note, the value is only meaningful
+	 * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set.
+	 */
+	if (type == HFSPLUS_FOLDER)
+		HFSPLUS_I(dst_dir)->folder_count++;
+
 	dst_dir->i_mtime = dst_dir->i_ctime = CURRENT_TIME_SEC;
 
 	/* finally remove the old entry */
@@ -405,6 +432,14 @@ int hfsplus_rename_cat(u32 cnid,
 	if (err)
 		goto out;
 	src_dir->i_size--;
+
+	/*
+	 * Decrement folder count. Note, the value is only meaningful
+	 * for folders with HFSPLUS_HAS_FOLDER_COUNT flag set.
+	 */
+	if (type == HFSPLUS_FOLDER)
+		HFSPLUS_I(src_dir)->folder_count--;
+
 	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..e1911fc 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -242,6 +242,8 @@ struct hfsplus_inode_info {
 	 */
 	sector_t fs_blocks;
 	u8 userflags;		/* BSD user file flags */
+	u32 folder_count;	/* Subfolder count if
+				 * HFSPLUS_HAS_FOLDER_COUNT is set */
 	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..61aeea8 100644
--- a/fs/hfsplus/hfsplus_raw.h
+++ b/fs/hfsplus/hfsplus_raw.h
@@ -261,7 +261,10 @@ struct hfsplus_cat_folder {
 	struct DInfo user_info;
 	struct DXInfo finder_info;
 	__be32 text_encoding;
-	u32 reserved;
+
+	/* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT flag is set.
+	 * Reserved otherwise. */
+	__be32 folder_count;
 } __packed;
 
 /* HFS file info (stolen from hfs.h) */
@@ -306,6 +309,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
 
 /* 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..1b56f50 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->folder_count = 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)->folder_count
+				 = be32_to_cpu(folder->folder_count);
+		}
 		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->folder_count
+				 = cpu_to_be32(HFSPLUS_I(inode)->folder_count);
+		}
 		hfs_bnode_write(fd.bnode, &entry, fd.entryoffset,
 					 sizeof(struct hfsplus_cat_folder));
 	} else if (HFSPLUS_IS_RSRC(inode)) {
-- 
1.7.10.2

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

[Vyacheslav Dubeyko]

Hi Sergei,

Thank you for the second version of the patch. It looks much better. But
it is easier to answer on the patch in the e-mail body. Anyway, you
should send the next (and, maybe, final) version of the patch in the
e-mail body.

>> 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.)

Please, use 80 symbol on the line rule and for patch's comment too.

>> diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
>> index 08846425b..e1911fc 100644
>> --- a/fs/hfsplus/hfsplus_fs.h
>> +++ b/fs/hfsplus/hfsplus_fs.h
>> @@ -242,6 +242,8 @@ struct hfsplus_inode_info {
>> 	 */
>> 	sector_t fs_blocks;
>> 	u8 userflags;		/* BSD user file flags */
>> +	u32 folder_count;	/* Subfolder count if
>> +				 * HFSPLUS_HAS_FOLDER_COUNT is set */

I suggest to change comment here: "Subfolders count (HFSX only)".
And it makes sense to add more detailed comment for
HFSPLUS_HAS_FOLDER_COUNT flag.

>> diff --git a/fs/hfsplus/hfsplus_raw.h b/fs/hfsplus/hfsplus_raw.h
>> index 8ffb3a8..61aeea8 100644
>> --- a/fs/hfsplus/hfsplus_raw.h
>> +++ b/fs/hfsplus/hfsplus_raw.h
>> @@ -261,7 +261,10 @@ struct hfsplus_cat_folder {
>> 	struct DInfo user_info;
>> 	struct DXInfo finder_info;
>> 	__be32 text_encoding;
>> -	u32 reserved;
>> +
>> +	/* Number of subfolders when HFSPLUS_HAS_FOLDER_COUNT flag is set.
>> +	 * Reserved otherwise. */
>> +	__be32 folder_count;

What about shorter name as "subfolders" for both fields in struct
hfsplus_inode_info and in struct hfsplus_cat_folder? I suppose it will
be more informative and correct.

I suggest to change comment here: "Subfolders count (HFSX only).
Reserved otherwise.". And it makes sense to add more detailed comment
for HFSPLUS_HAS_FOLDER_COUNT flag.

>> diff --git a/fs/hfsplus/catalog.c b/fs/hfsplus/catalog.c
>> index 968ce41..04228d6 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);

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.

Thanks,
Vyacheslav Dubeyko.

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

[Sergei Antonov]

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.

The new patch follows. With your suggestions included and two
increment/decrement functions to avoid code duplication.

-- 
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, deleteion, and rename.

Signed-off-by: Sergei Antonov <saproj@gmail.com>
---
 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++;
+}
+
+/*
+ * 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--;
+}
+
 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);
 	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);
 	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) */
 	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+. */
 } __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) */
 
 /* 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);
+		}
 		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

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

[Vyacheslav Dubeyko]

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@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, deleteion, and rename.
> 
> Signed-off-by: Sergei Antonov <saproj@gmail.com>
> ---
>  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.

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

[Sergei Antonov]

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

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

[Vyacheslav Dubeyko]

Hi Sergei,

On Thu, 2014-02-06 at 18:27 +0100, Sergei Antonov wrote:

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

Ok. Thank you.

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

Ok. I don't insist. But I think that using inline keyword is a good way.
Also, I don't think that macros is a way to the hell. :)

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

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

I meant only that you have done in the patch yet. But, maybe, it makes
sense to check on integer overflow or folders count limitation for HFS+.

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

I think that "subfolders count" is correct. But if you think in other
way then I don't insist. It is not critical. But I suppose that we have
necessity in any count when we have many items instead of one.

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

Ok. Now I see that it is enough.

> 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).
> 

It makes sense if this flag will be used in drivers functionality. I
think so.

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

Ok. Now we have elaborated a good state of the patch, from my viewpoint.
I suggest to prepare and to send last version of the patch. Please, add
in Cc Al Viro <viro@zeniv.linux.org.uk>, ChristophHellwig
<hch@infradead.org>, Andrew Morton <akpm@linux-foundation.org>. I will
glad to add my Reviewed-by because current state of the patch looks good
for me.

Thanks,
Vyacheslav Dubeyko.