linux-fsdevel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 3/3] hfsplus: implement attributes file creation functionality
@ 2013-09-20 10:04 Vyacheslav Dubeyko
  2013-09-20 22:30 ` Hin-Tak Leung
  2013-09-25 23:02 ` Andrew Morton
  0 siblings, 2 replies; 6+ messages in thread
From: Vyacheslav Dubeyko @ 2013-09-20 10:04 UTC (permalink / raw)
  To: Linux FS devel list
  Cc: Al Viro, Christoph Hellwig, Hin-Tak Leung, Andrew Morton

From: Vyacheslav Dubeyko <slava@dubeyko.com>
Subject: [PATCH 3/3] hfsplus: implement attributes file creation functionality

This patch implements functionality of creation AttributesFile
metadata file on HFS+ volume in the case of absence of it.

It makes trying to open AttributesFile's B-tree during mount of
HFS+ volume. If HFS+ volume hasn't AttributesFile then a pointer
on AttributesFile's B-tree keeps as NULL. Thereby, when it is
discovered absence of AttributesFile on HFS+ volume in the begin
of xattr creation operation then AttributesFile will be created.

The creation of AttributesFile will have success in the case of
availability (2 * clump) free blocks on HFS+ volume. Otherwise,
creation operation is ended with error (-ENOSPC).

Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>
CC: Al Viro <viro@zeniv.linux.org.uk>
CC: Christoph Hellwig <hch@infradead.org>
CC: Hin-Tak Leung <htl10@users.sourceforge.net>
---
 fs/hfsplus/hfsplus_fs.h |    9 ++++
 fs/hfsplus/super.c      |    2 +
 fs/hfsplus/xattr.c      |  128 ++++++++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 137 insertions(+), 2 deletions(-)

diff --git a/fs/hfsplus/hfsplus_fs.h b/fs/hfsplus/hfsplus_fs.h
index 1e36f18..08846425b 100644
--- a/fs/hfsplus/hfsplus_fs.h
+++ b/fs/hfsplus/hfsplus_fs.h
@@ -127,6 +127,14 @@ struct hfs_bnode {
 #define HFS_BNODE_DELETED	4
 
 /*
+ * Attributes file states
+ */
+#define HFSPLUS_EMPTY_ATTR_TREE		0
+#define HFSPLUS_CREATING_ATTR_TREE	1
+#define HFSPLUS_VALID_ATTR_TREE		2
+#define HFSPLUS_FAILED_ATTR_TREE	3
+
+/*
  * HFS+ superblock info (built from Volume Header on disk)
  */
 
@@ -141,6 +149,7 @@ struct hfsplus_sb_info {
 	struct hfs_btree *ext_tree;
 	struct hfs_btree *cat_tree;
 	struct hfs_btree *attr_tree;
+	atomic_t attr_tree_state;
 	struct inode *alloc_file;
 	struct inode *hidden_dir;
 	struct nls_table *nls;
diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
index 4c4d142..80875aa 100644
--- a/fs/hfsplus/super.c
+++ b/fs/hfsplus/super.c
@@ -474,12 +474,14 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
 		pr_err("failed to load catalog file\n");
 		goto out_close_ext_tree;
 	}
+	atomic_set(&sbi->attr_tree_state, HFSPLUS_EMPTY_ATTR_TREE);
 	if (vhdr->attr_file.total_blocks != 0) {
 		sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
 		if (!sbi->attr_tree) {
 			pr_err("failed to load attributes file\n");
 			goto out_close_cat_tree;
 		}
+		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
 	}
 	sb->s_xattr = hfsplus_xattr_handlers;
 
diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
index 31f70f5..c3d829c 100644
--- a/fs/hfsplus/xattr.c
+++ b/fs/hfsplus/xattr.c
@@ -190,6 +190,129 @@ static void hfsplus_init_header_node(struct inode *attr_file,
 	SETOFFSET(buf, node_size, offset, 4);
 }
 
+static int hfsplus_create_attributes_file(struct super_block *sb)
+{
+	int err = 0;
+	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
+	struct inode *attr_file;
+	struct hfsplus_inode_info *hip;
+	u32 clump_size;
+	u16 node_size = HFSPLUS_ATTR_TREE_NODE_SIZE;
+	char *buf;
+	int index, written;
+	struct address_space *mapping;
+	struct page *page;
+	int old_state = HFSPLUS_EMPTY_ATTR_TREE;
+
+	hfs_dbg(ATTR_MOD, "create_attr_file: ino %d\n", HFSPLUS_ATTR_CNID);
+
+check_attr_tree_state_again:
+	switch (atomic_read(&sbi->attr_tree_state)) {
+	case HFSPLUS_EMPTY_ATTR_TREE:
+		if (old_state != atomic_cmpxchg(&sbi->attr_tree_state,
+						old_state,
+						HFSPLUS_CREATING_ATTR_TREE))
+			goto check_attr_tree_state_again;
+		break;
+	case HFSPLUS_CREATING_ATTR_TREE:
+		schedule_timeout_uninterruptible(HZ);
+		goto check_attr_tree_state_again;
+		break;
+	case HFSPLUS_VALID_ATTR_TREE:
+		return 0;
+	case HFSPLUS_FAILED_ATTR_TREE:
+		return -EOPNOTSUPP;
+	default:
+		BUG();
+	}
+
+	attr_file = hfsplus_iget(sb, HFSPLUS_ATTR_CNID);
+	if (IS_ERR(attr_file)) {
+		pr_err("failed to load attributes file\n");
+		return PTR_ERR(attr_file);
+	}
+
+	BUG_ON(i_size_read(attr_file) != 0);
+
+	hip = HFSPLUS_I(attr_file);
+
+	clump_size = hfsplus_calc_btree_clump_size(sb->s_blocksize,
+						    node_size,
+						    sbi->sect_count,
+						    HFSPLUS_ATTR_CNID);
+
+	mutex_lock(&hip->extents_lock);
+	hip->clump_blocks = clump_size >> sbi->alloc_blksz_shift;
+	mutex_unlock(&hip->extents_lock);
+
+	if (sbi->free_blocks <= (hip->clump_blocks << 1)) {
+		err = -ENOSPC;
+		goto end_attr_file_creation;
+	}
+
+	while (hip->alloc_blocks < hip->clump_blocks) {
+		err = hfsplus_file_extend(attr_file);
+		if (unlikely(err)) {
+			pr_err("failed to extend attributes file\n");
+			goto end_attr_file_creation;
+		}
+		hip->phys_size = attr_file->i_size =
+			(loff_t)hip->alloc_blocks << sbi->alloc_blksz_shift;
+		hip->fs_blocks = hip->alloc_blocks << sbi->fs_shift;
+		inode_set_bytes(attr_file, attr_file->i_size);
+	}
+
+	buf = kzalloc(node_size, GFP_NOFS);
+	if (!buf) {
+		pr_err("failed to allocate memory for header node\n");
+		err = -ENOMEM;
+		goto end_attr_file_creation;
+	}
+
+	hfsplus_init_header_node(attr_file, clump_size, buf, node_size);
+
+	mapping = attr_file->i_mapping;
+
+	index = 0;
+	written = 0;
+	for (; written < node_size; index++, written += PAGE_CACHE_SIZE) {
+		void *kaddr;
+
+		page = read_mapping_page(mapping, index, NULL);
+		if (IS_ERR(page))
+			goto failed_header_node_init;
+
+		kaddr = kmap_atomic(page);
+		memcpy(kaddr, buf + written,
+			min_t(size_t, PAGE_CACHE_SIZE, node_size - written));
+		kunmap_atomic(kaddr);
+
+		set_page_dirty(page);
+		page_cache_release(page);
+	}
+
+	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
+
+	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
+	if (!sbi->attr_tree)
+		pr_err("failed to load attributes file\n");
+
+failed_header_node_init:
+	kfree(buf);
+
+end_attr_file_creation:
+	iput(attr_file);
+
+	if (!err)
+		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
+	else if (err == -ENOSPC)
+		atomic_set(&sbi->attr_tree_state, HFSPLUS_EMPTY_ATTR_TREE);
+	else
+		atomic_set(&sbi->attr_tree_state, HFSPLUS_FAILED_ATTR_TREE);
+
+	return err;
+}
+
 int __hfsplus_setxattr(struct inode *inode, const char *name,
 			const void *value, size_t size, int flags)
 {
@@ -274,8 +397,9 @@ int __hfsplus_setxattr(struct inode *inode, const char *name,
 	}
 
 	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
-		err = -EOPNOTSUPP;
-		goto end_setxattr;
+		err = hfsplus_create_attributes_file(inode->i_sb);
+		if (unlikely(err))
+			goto end_setxattr;
 	}
 
 	if (hfsplus_attr_exists(inode, name)) {
-- 
1.7.9.5




^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] hfsplus: implement attributes file creation functionality
  2013-09-20 10:04 [PATCH 3/3] hfsplus: implement attributes file creation functionality Vyacheslav Dubeyko
@ 2013-09-20 22:30 ` Hin-Tak Leung
  2013-09-23  6:33   ` Vyacheslav Dubeyko
  2013-09-25 23:02 ` Andrew Morton
  1 sibling, 1 reply; 6+ messages in thread
From: Hin-Tak Leung @ 2013-09-20 22:30 UTC (permalink / raw)
  To: Linux FS devel list, slava; +Cc: Al Viro, Christoph Hellwig, Andrew Morton


--------------------------------------------
On Fri, 20/9/13, Vyacheslav Dubeyko <slava@dubeyko.com> wrote:

 Subject: [PATCH 3/3] hfsplus: implement attributes file creation functionality
 To: "Linux FS devel list" <linux-fsdevel@vger.kernel.org>
 Cc: "Al Viro" <viro@zeniv.linux.org.uk>, "Christoph Hellwig" <hch@infradead.org>, "Hin-Tak Leung" <htl10@users.sourceforge.net>, "Andrew Morton" <akpm@linux-foundation.org>
 Date: Friday, 20 September, 2013, 11:04
 
 From: Vyacheslav Dubeyko <slava@dubeyko.com>
 Subject: [PATCH 3/3] hfsplus: implement attributes file
 creation functionality
 
 This patch implements functionality of creation
 AttributesFile
 metadata file on HFS+ volume in the case of absence of it.
 
 It makes trying to open AttributesFile's B-tree during mount
 of
 HFS+ volume. If HFS+ volume hasn't AttributesFile then a
 pointer
 on AttributesFile's B-tree keeps as NULL. Thereby, when it
 is
 discovered absence of AttributesFile on HFS+ volume in the
 begin
 of xattr creation operation then AttributesFile will be
 created.
 
 The creation of AttributesFile will have success in the case
 of
 availability (2 * clump) free blocks on HFS+ volume.
 Otherwise,
 creation operation is ended with error (-ENOSPC).
 
 Signed-off-by: Vyacheslav Dubeyko <slava@dubeyko.com>
 CC: Al Viro <viro@zeniv.linux.org.uk>
 CC: Christoph Hellwig <hch@infradead.org>
 CC: Hin-Tak Leung <htl10@users.sourceforge.net>
 ---

Acked-by: Hin-Tak Leung <htl10@users.sourceforge.net>

The changelog isn't quite correct/precise. According to the code (my reading and
understanding of it), The AttributesFile is created on first need, i.e. when the first-ever
attribute is set. The changelog sounds as if an empty one might be created
if missing, on mount. The latter would be somewhat undesirable, as a user might
just keep a particular disk quite full but contain entirely of plain no-attribute files, and
have no need of a big AttributesFile ever, and suddenly find a disk throwing an previously-not-seen
error on use.

Usage on linux would mostly be standalone xsetattr; the other common use would be
the copy of a file plus its attribute, from another fs which supports this, to an hfs+ fs without
a  AttributesFile yet. How does hfs+ under Mac OS behaves vs current Linux vs patched Linux?
(silently dropping attribute/drop attribute with warning/error/etc ?)

Hin-Tak
 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] hfsplus: implement attributes file creation functionality
  2013-09-20 22:30 ` Hin-Tak Leung
@ 2013-09-23  6:33   ` Vyacheslav Dubeyko
  0 siblings, 0 replies; 6+ messages in thread
From: Vyacheslav Dubeyko @ 2013-09-23  6:33 UTC (permalink / raw)
  To: htl10; +Cc: Linux FS devel list, Al Viro, Christoph Hellwig, Andrew Morton

On Fri, 2013-09-20 at 23:30 +0100, Hin-Tak Leung wrote:
> --------------------------------------------

[snip]
>  
>  This patch implements functionality of creation
>  AttributesFile metadata file on HFS+ volume in the case of absence of it.
>  
>  It makes trying to open AttributesFile's B-tree during mount
>  of HFS+ volume. If HFS+ volume hasn't AttributesFile then a
>  pointer on AttributesFile's B-tree keeps as NULL. Thereby, when it
>  is discovered absence of AttributesFile on HFS+ volume in the
>  begin of xattr creation operation then AttributesFile will be
>  created.
>  
>  The creation of AttributesFile will have success in the case
>  of availability (2 * clump) free blocks on HFS+ volume.
>  Otherwise, creation operation is ended with error (-ENOSPC).

[snip]
> 
> The changelog isn't quite correct/precise. According to the code (my reading and
> understanding of it), The AttributesFile is created on first need, i.e. when the first-ever
> attribute is set. The changelog sounds as if an empty one might be created
> if missing, on mount. The latter would be somewhat undesirable, as a user might
> just keep a particular disk quite full but contain entirely of plain no-attribute files, and
> have no need of a big AttributesFile ever, and suddenly find a disk throwing an previously-not-seen
> error on use.
> 

I don't think that description of the patch set is inaccurate. And I
misunderstand how you can conclude from path set description that
AttributesFile is created during mount operation. The patch description
states that HFS+ driver tries to open AttributesFile's tree by means of
hfs_btree_open() call (but such call takes place only if total blocks of
AttributesFile doesn't equal to zero). So, hfs_btree_open() method
doesn't contain any code of creation AttributesFile. And, finally,
description of the patch states that AttributesFile creation
functionality can be called in the begin of operation of extended
attribute (xattr) creation, namely, in __hfsplus_setxattr() method.

> Usage on linux would mostly be standalone xsetattr; the other common use would be
> the copy of a file plus its attribute, from another fs which supports this, to an hfs+ fs without
> a  AttributesFile yet. How does hfs+ under Mac OS behaves vs current Linux vs patched Linux?
> (silently dropping attribute/drop attribute with warning/error/etc ?)

As far as I can judge, Mac OS X doesn't copy xattrs of files during
copying operation. And, as far as I can see, Linux does it in the same
way. Anyway, a file system driver should have some set of method for
xattrs support. And HFS+ driver has all necessary methods (for xattrs
support) as implemented.

With the best regards,
Vyacheslav Dubeyko.



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] hfsplus: implement attributes file creation functionality
@ 2013-09-24 11:40 Hin-Tak Leung
  2013-09-24 12:04 ` Vyacheslav Dubeyko
  0 siblings, 1 reply; 6+ messages in thread
From: Hin-Tak Leung @ 2013-09-24 11:40 UTC (permalink / raw)
  To: slava; +Cc: linux-fsdevel, viro, hch, akpm

------------------------------
On Mon, Sep 23, 2013 07:33 BST Vyacheslav Dubeyko wrote:

>On Fri, 2013-09-20 at 23:30 +0100, Hin-Tak Leung wrote:
>> --------------------------------------------
>
>[snip]
>>  
>>  This patch implements functionality of creation
>>  AttributesFile metadata file on HFS+ volume in the case of absence of it.
>>  
>>  It makes trying to open AttributesFile's B-tree during mount
>>  of HFS+ volume. If HFS+ volume hasn't AttributesFile then a
>>  pointer on AttributesFile's B-tree keeps as NULL. Thereby, when it
>>  is discovered absence of AttributesFile on HFS+ volume in the
>>  begin of xattr creation operation then AttributesFile will be
>>  created.
>>  
>>  The creation of AttributesFile will have success in the case
>>  of availability (2 * clump) free blocks on HFS+ volume.
>>  Otherwise, creation operation is ended with error (-ENOSPC).
>
>[snip]
>> 
>> The changelog isn't quite correct/precise. According to the code (my reading and
>> understanding of it), The AttributesFile is created on first need, i.e. when the first-ever
>> attribute is set. The changelog sounds as if an empty one might be created
>> if missing, on mount. The latter would be somewhat undesirable, as a user might
>> just keep a particular disk quite full but contain entirely of plain no-attribute files, and
>> have no need of a big AttributesFile ever, and suddenly find a disk throwing an previously-not-seen
>> error on use.
>> 
>
>I don't think that description of the patch set is inaccurate. And I
>misunderstand how you can conclude from path set description that
>AttributesFile is created during mount operation. The patch description
>states that HFS+ driver tries to open AttributesFile's tree by means of
>hfs_btree_open() call (but such call takes place only if total blocks of
>AttributesFile doesn't equal to zero). So, hfs_btree_open() method
>doesn't contain any code of creation AttributesFile. And, finally,
>description of the patch states that AttributesFile creation
>functionality can be called in the begin of operation of extended
>attribute (xattr) creation, namely, in __hfsplus_setxattr() method.

"...during mount...", first sentence, 2nd paragraph.

>> Usage on linux would mostly be standalone xsetattr; the other common use would be
>> the copy of a file plus its attribute, from another fs which supports this, to an hfs+ fs without
>> a  AttributesFile yet. How does hfs+ under Mac OS behaves vs current Linux vs patched Linux?
>> (silently dropping attribute/drop attribute with warning/error/etc ?)
>
>As far as I can judge, Mac OS X doesn't copy xattrs of files during
>copying operation. And, as far as I can see, Linux does it in the same
>way. Anyway, a file system driver should have some set of method for
>xattrs support. And HFS+ driver has all necessary methods (for xattrs
>support) as implemented.

I meant the generic user-invoked "copy" operation. AFAIK (first hand experience?), drag-and-drop
from Finder does copy some xattrs/rscs ; and I seem to remember have came across an enhancement
patch for rsync which does that too, which has either been merged upstream or shipped-as-patched.
While the command-line 'cp' does not do it by default, isn't there an option for doing it?

Hin-Tak

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] hfsplus: implement attributes file creation functionality
  2013-09-24 11:40 Hin-Tak Leung
@ 2013-09-24 12:04 ` Vyacheslav Dubeyko
  0 siblings, 0 replies; 6+ messages in thread
From: Vyacheslav Dubeyko @ 2013-09-24 12:04 UTC (permalink / raw)
  To: htl10; +Cc: linux-fsdevel, viro, hch, akpm


On Sep 24, 2013, at 3:40 PM, Hin-Tak Leung wrote:

> ------------------------------
> On Mon, Sep 23, 2013 07:33 BST Vyacheslav Dubeyko wrote:
> 
>> On Fri, 2013-09-20 at 23:30 +0100, Hin-Tak Leung wrote:
>>> --------------------------------------------
>> 
>> [snip]
>>>   
>>>   This patch implements functionality of creation
>>>   AttributesFile metadata file on HFS+ volume in the case of absence of it.
>>>   
>>>   It makes trying to open AttributesFile's B-tree during mount
>>>   of HFS+ volume. If HFS+ volume hasn't AttributesFile then a
>>>   pointer on AttributesFile's B-tree keeps as NULL. Thereby, when it
>>>   is discovered absence of AttributesFile on HFS+ volume in the
>>>   begin of xattr creation operation then AttributesFile will be
>>>   created.
>>>   
>>>   The creation of AttributesFile will have success in the case
>>>   of availability (2 * clump) free blocks on HFS+ volume.
>>>   Otherwise, creation operation is ended with error (-ENOSPC).
>> 

[snip]
> 
> "...during mount...", first sentence, 2nd paragraph.
> 

Where do you see statement about trying to create AttributesFile during mount
in first sentence of 2nd paragraph? This statement tells about opening B-tree
during mount. But opening is not creation, as far as I can see.

>>> Usage on linux would mostly be standalone xsetattr; the other common use would be
>>> the copy of a file plus its attribute, from another fs which supports this, to an hfs+ fs without
>>> a  AttributesFile yet. How does hfs+ under Mac OS behaves vs current Linux vs patched Linux?
>>> (silently dropping attribute/drop attribute with warning/error/etc ?)
>> 
>> As far as I can judge, Mac OS X doesn't copy xattrs of files during
>> copying operation. And, as far as I can see, Linux does it in the same
>> way. Anyway, a file system driver should have some set of method for
>> xattrs support. And HFS+ driver has all necessary methods (for xattrs
>> support) as implemented.
> 
> I meant the generic user-invoked "copy" operation. AFAIK (first hand experience?), drag-and-drop
> from Finder does copy some xattrs/rscs ; and I seem to remember have came across an enhancement
> patch for rsync which does that too, which has either been merged upstream or shipped-as-patched.
> While the command-line 'cp' does not do it by default, isn't there an option for doing it?
> 

This is question to "cp" utility. Finder is complex application in user-space. So, it can
create xattrs during copying operation. But HFS+ driver implements all necessary methods
(listxattr, getxattr, setxattr, removexattr) that it is called by VFS layer.

With the best regards,
Vyacheslav Dubeyko.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] hfsplus: implement attributes file creation functionality
  2013-09-20 10:04 [PATCH 3/3] hfsplus: implement attributes file creation functionality Vyacheslav Dubeyko
  2013-09-20 22:30 ` Hin-Tak Leung
@ 2013-09-25 23:02 ` Andrew Morton
  1 sibling, 0 replies; 6+ messages in thread
From: Andrew Morton @ 2013-09-25 23:02 UTC (permalink / raw)
  To: slava; +Cc: Linux FS devel list, Al Viro, Christoph Hellwig, Hin-Tak Leung

On Fri, 20 Sep 2013 14:04:22 +0400 Vyacheslav Dubeyko <slava@dubeyko.com> wrote:

> From: Vyacheslav Dubeyko <slava@dubeyko.com>
> Subject: [PATCH 3/3] hfsplus: implement attributes file creation functionality
> 
> This patch implements functionality of creation AttributesFile
> metadata file on HFS+ volume in the case of absence of it.
> 
> It makes trying to open AttributesFile's B-tree during mount of
> HFS+ volume. If HFS+ volume hasn't AttributesFile then a pointer
> on AttributesFile's B-tree keeps as NULL. Thereby, when it is
> discovered absence of AttributesFile on HFS+ volume in the begin
> of xattr creation operation then AttributesFile will be created.
> 
> The creation of AttributesFile will have success in the case of
> availability (2 * clump) free blocks on HFS+ volume. Otherwise,
> creation operation is ended with error (-ENOSPC).
> 
> ...
>
> --- a/fs/hfsplus/hfsplus_fs.h
> +++ b/fs/hfsplus/hfsplus_fs.h
> @@ -127,6 +127,14 @@ struct hfs_bnode {
>  #define HFS_BNODE_DELETED	4
>  
>  /*
> + * Attributes file states
> + */
> +#define HFSPLUS_EMPTY_ATTR_TREE		0
> +#define HFSPLUS_CREATING_ATTR_TREE	1
> +#define HFSPLUS_VALID_ATTR_TREE		2
> +#define HFSPLUS_FAILED_ATTR_TREE	3
> +
> +/*
>   * HFS+ superblock info (built from Volume Header on disk)
>   */
>  
> @@ -141,6 +149,7 @@ struct hfsplus_sb_info {
>  	struct hfs_btree *ext_tree;
>  	struct hfs_btree *cat_tree;
>  	struct hfs_btree *attr_tree;
> +	atomic_t attr_tree_state;
>  	struct inode *alloc_file;
>  	struct inode *hidden_dir;
>  	struct nls_table *nls;
> diff --git a/fs/hfsplus/super.c b/fs/hfsplus/super.c
> index 4c4d142..80875aa 100644
> --- a/fs/hfsplus/super.c
> +++ b/fs/hfsplus/super.c
> @@ -474,12 +474,14 @@ static int hfsplus_fill_super(struct super_block *sb, void *data, int silent)
>  		pr_err("failed to load catalog file\n");
>  		goto out_close_ext_tree;
>  	}
> +	atomic_set(&sbi->attr_tree_state, HFSPLUS_EMPTY_ATTR_TREE);
>  	if (vhdr->attr_file.total_blocks != 0) {
>  		sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
>  		if (!sbi->attr_tree) {
>  			pr_err("failed to load attributes file\n");
>  			goto out_close_cat_tree;
>  		}
> +		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
>  	}
>  	sb->s_xattr = hfsplus_xattr_handlers;
>  
> diff --git a/fs/hfsplus/xattr.c b/fs/hfsplus/xattr.c
> index 31f70f5..c3d829c 100644
> --- a/fs/hfsplus/xattr.c
> +++ b/fs/hfsplus/xattr.c
> @@ -190,6 +190,129 @@ static void hfsplus_init_header_node(struct inode *attr_file,
>  	SETOFFSET(buf, node_size, offset, 4);
>  }
>  
> +static int hfsplus_create_attributes_file(struct super_block *sb)
> +{
> +	int err = 0;
> +	struct hfsplus_sb_info *sbi = HFSPLUS_SB(sb);
> +	struct inode *attr_file;
> +	struct hfsplus_inode_info *hip;
> +	u32 clump_size;
> +	u16 node_size = HFSPLUS_ATTR_TREE_NODE_SIZE;
> +	char *buf;
> +	int index, written;
> +	struct address_space *mapping;
> +	struct page *page;
> +	int old_state = HFSPLUS_EMPTY_ATTR_TREE;
> +
> +	hfs_dbg(ATTR_MOD, "create_attr_file: ino %d\n", HFSPLUS_ATTR_CNID);
> +
> +check_attr_tree_state_again:
> +	switch (atomic_read(&sbi->attr_tree_state)) {
> +	case HFSPLUS_EMPTY_ATTR_TREE:
> +		if (old_state != atomic_cmpxchg(&sbi->attr_tree_state,
> +						old_state,
> +						HFSPLUS_CREATING_ATTR_TREE))
> +			goto check_attr_tree_state_again;
> +		break;
> +	case HFSPLUS_CREATING_ATTR_TREE:
> +		schedule_timeout_uninterruptible(HZ);

Use of msleep(1000) would be preferred.

Also, what on earth is this delay doing in the middle of filesystem
code??  It should either be removed in favour of a proper fix, or very
clearly explained in code comments.  Simply sticking it in there
unexplained will not fly!

> +		goto check_attr_tree_state_again;
> +		break;

The `break' is obviously unreachable and I suggest simply removing it.

> +	case HFSPLUS_VALID_ATTR_TREE:
> +		return 0;
> +	case HFSPLUS_FAILED_ATTR_TREE:
> +		return -EOPNOTSUPP;
> +	default:
> +		BUG();
> +	}
> +
> +	attr_file = hfsplus_iget(sb, HFSPLUS_ATTR_CNID);
> +	if (IS_ERR(attr_file)) {
> +		pr_err("failed to load attributes file\n");
> +		return PTR_ERR(attr_file);
> +	}
> +
> +	BUG_ON(i_size_read(attr_file) != 0);
> +
> +	hip = HFSPLUS_I(attr_file);
> +
> +	clump_size = hfsplus_calc_btree_clump_size(sb->s_blocksize,
> +						    node_size,
> +						    sbi->sect_count,
> +						    HFSPLUS_ATTR_CNID);
> +
> +	mutex_lock(&hip->extents_lock);
> +	hip->clump_blocks = clump_size >> sbi->alloc_blksz_shift;
> +	mutex_unlock(&hip->extents_lock);
> +
> +	if (sbi->free_blocks <= (hip->clump_blocks << 1)) {
> +		err = -ENOSPC;
> +		goto end_attr_file_creation;
> +	}
> +
> +	while (hip->alloc_blocks < hip->clump_blocks) {
> +		err = hfsplus_file_extend(attr_file);
> +		if (unlikely(err)) {
> +			pr_err("failed to extend attributes file\n");
> +			goto end_attr_file_creation;
> +		}
> +		hip->phys_size = attr_file->i_size =
> +			(loff_t)hip->alloc_blocks << sbi->alloc_blksz_shift;
> +		hip->fs_blocks = hip->alloc_blocks << sbi->fs_shift;
> +		inode_set_bytes(attr_file, attr_file->i_size);
> +	}
> +
> +	buf = kzalloc(node_size, GFP_NOFS);
> +	if (!buf) {
> +		pr_err("failed to allocate memory for header node\n");
> +		err = -ENOMEM;
> +		goto end_attr_file_creation;
> +	}
> +
> +	hfsplus_init_header_node(attr_file, clump_size, buf, node_size);
> +
> +	mapping = attr_file->i_mapping;
> +
> +	index = 0;
> +	written = 0;
> +	for (; written < node_size; index++, written += PAGE_CACHE_SIZE) {
> +		void *kaddr;
> +
> +		page = read_mapping_page(mapping, index, NULL);
> +		if (IS_ERR(page))
> +			goto failed_header_node_init;

Forgot to set `err' here, which looks to me like a bug.

> +		kaddr = kmap_atomic(page);
> +		memcpy(kaddr, buf + written,
> +			min_t(size_t, PAGE_CACHE_SIZE, node_size - written));
> +		kunmap_atomic(kaddr);
> +
> +		set_page_dirty(page);
> +		page_cache_release(page);
> +	}
> +
> +	hfsplus_mark_inode_dirty(attr_file, HFSPLUS_I_ATTR_DIRTY);
> +
> +	sbi->attr_tree = hfs_btree_open(sb, HFSPLUS_ATTR_CNID);
> +	if (!sbi->attr_tree)
> +		pr_err("failed to load attributes file\n");
> +
> +failed_header_node_init:
> +	kfree(buf);
> +
> +end_attr_file_creation:
> +	iput(attr_file);
> +
> +	if (!err)
> +		atomic_set(&sbi->attr_tree_state, HFSPLUS_VALID_ATTR_TREE);
> +	else if (err == -ENOSPC)
> +		atomic_set(&sbi->attr_tree_state, HFSPLUS_EMPTY_ATTR_TREE);
> +	else
> +		atomic_set(&sbi->attr_tree_state, HFSPLUS_FAILED_ATTR_TREE);
> +
> +	return err;
> +}
> +
>  int __hfsplus_setxattr(struct inode *inode, const char *name,
>  			const void *value, size_t size, int flags)
>  {
> @@ -274,8 +397,9 @@ int __hfsplus_setxattr(struct inode *inode, const char *name,
>  	}
>  
>  	if (!HFSPLUS_SB(inode->i_sb)->attr_tree) {
> -		err = -EOPNOTSUPP;
> -		goto end_setxattr;
> +		err = hfsplus_create_attributes_file(inode->i_sb);
> +		if (unlikely(err))
> +			goto end_setxattr;
>  	}
>  
>  	if (hfsplus_attr_exists(inode, name)) {


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2013-09-25 23:02 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-20 10:04 [PATCH 3/3] hfsplus: implement attributes file creation functionality Vyacheslav Dubeyko
2013-09-20 22:30 ` Hin-Tak Leung
2013-09-23  6:33   ` Vyacheslav Dubeyko
2013-09-25 23:02 ` Andrew Morton
  -- strict thread matches above, loose matches on Subject: below --
2013-09-24 11:40 Hin-Tak Leung
2013-09-24 12:04 ` Vyacheslav Dubeyko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).