On 4 February 2014 08:10, Vyacheslav Dubeyko <slava@xxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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.
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@xxxxxxxxx>
---
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
