Re: [PATCH 3/3] hfsplus: implement attributes file creation functionality

[Andrew Morton <akpm@linux-foundation.org>]

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