[PATCH] Btrfs: implement inode_operations callback tmpfile

[PATCH] Btrfs: implement inode_operations callback tmpfile

[Filipe David Borba Manana]

This implements the tmpfile callback of struct inode_operations, introduced
in the linux kernel 3.11 [1], and implemented already by some filesystems.

Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---
 fs/btrfs/inode.c | 120 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 100 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0ec8766..1a38ec7 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5522,6 +5522,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	struct btrfs_inode_ref *ref;
 	struct btrfs_key key[2];
 	u32 sizes[2];
+	int nitems = name ? 2 : 1;
 	unsigned long ptr;
 	int ret;
 
@@ -5541,7 +5542,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	 */
 	inode->i_ino = objectid;
 
-	if (dir) {
+	if (dir && name) {
 		trace_btrfs_inode_request(dir);
 
 		ret = btrfs_set_inode_index(dir, index);
@@ -5550,6 +5551,8 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 			iput(inode);
 			return ERR_PTR(ret);
 		}
+	} else if (dir) {
+		*index = 0;
 	}
 	/*
 	 * index_cnt is ignored for everything but a dir,
@@ -5574,21 +5577,24 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	btrfs_set_key_type(&key[0], BTRFS_INODE_ITEM_KEY);
 	key[0].offset = 0;
 
-	/*
-	 * Start new inodes with an inode_ref. This is slightly more
-	 * efficient for small numbers of hard links since they will
-	 * be packed into one item. Extended refs will kick in if we
-	 * add more hard links than can fit in the ref item.
-	 */
-	key[1].objectid = objectid;
-	btrfs_set_key_type(&key[1], BTRFS_INODE_REF_KEY);
-	key[1].offset = ref_objectid;
-
 	sizes[0] = sizeof(struct btrfs_inode_item);
-	sizes[1] = name_len + sizeof(*ref);
+
+	if (name) {
+		/*
+		 * Start new inodes with an inode_ref. This is slightly more
+		 * efficient for small numbers of hard links since they will
+		 * be packed into one item. Extended refs will kick in if we
+		 * add more hard links than can fit in the ref item.
+		 */
+		key[1].objectid = objectid;
+		btrfs_set_key_type(&key[1], BTRFS_INODE_REF_KEY);
+		key[1].offset = ref_objectid;
+
+		sizes[1] = name_len + sizeof(*ref);
+	}
 
 	path->leave_spinning = 1;
-	ret = btrfs_insert_empty_items(trans, root, path, key, sizes, 2);
+	ret = btrfs_insert_empty_items(trans, root, path, key, sizes, nitems);
 	if (ret != 0)
 		goto fail;
 
@@ -5601,12 +5607,14 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 			     sizeof(*inode_item));
 	fill_inode_item(trans, path->nodes[0], inode_item, inode);
 
-	ref = btrfs_item_ptr(path->nodes[0], path->slots[0] + 1,
-			     struct btrfs_inode_ref);
-	btrfs_set_inode_ref_name_len(path->nodes[0], ref, name_len);
-	btrfs_set_inode_ref_index(path->nodes[0], ref, *index);
-	ptr = (unsigned long)(ref + 1);
-	write_extent_buffer(path->nodes[0], name, ptr, name_len);
+	if (name) {
+		ref = btrfs_item_ptr(path->nodes[0], path->slots[0] + 1,
+				     struct btrfs_inode_ref);
+		btrfs_set_inode_ref_name_len(path->nodes[0], ref, name_len);
+		btrfs_set_inode_ref_index(path->nodes[0], ref, *index);
+		ptr = (unsigned long)(ref + 1);
+		write_extent_buffer(path->nodes[0], name, ptr, name_len);
+	}
 
 	btrfs_mark_buffer_dirty(path->nodes[0]);
 	btrfs_free_path(path);
@@ -5642,7 +5650,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 
 	return inode;
 fail:
-	if (dir)
+	if (dir && name)
 		BTRFS_I(dir)->index_cnt--;
 	btrfs_free_path(path);
 	iput(inode);
@@ -5927,6 +5935,15 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
 		err = btrfs_update_inode(trans, root, inode);
 		if (err)
 			goto fail;
+		if (inode->i_nlink == 1) {
+			/*
+			 * If new hard link count is 1, it's a file created
+			 * with open(2) O_TMPFILE flag.
+			 */
+			err = btrfs_orphan_del(trans, inode);
+			if (err)
+				goto fail;
+		}
 		d_instantiate(dentry, inode);
 		btrfs_log_new_name(trans, inode, NULL, parent);
 	}
@@ -8858,6 +8875,68 @@ static int btrfs_permission(struct inode *inode, int mask)
 	return generic_permission(inode, mask);
 }
 
+static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
+{
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *root = BTRFS_I(dir)->root;
+	struct inode *inode = NULL;
+	u64 objectid;
+	u64 index;
+	int ret = 0;
+
+	/*
+	 * 2 for inode item and ref
+	 * 2 for dir items
+	 * 1 for xattr if selinux is on
+	 */
+	trans = btrfs_start_transaction(root, 5);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	ret = btrfs_find_free_ino(root, &objectid);
+	if (ret)
+		goto out;
+
+	inode = btrfs_new_inode(trans, root, dir, NULL, 0,
+				btrfs_ino(dir), objectid, mode, &index);
+	if (IS_ERR(inode)) {
+		ret = PTR_ERR(inode);
+		inode = NULL;
+		goto out;
+	}
+
+	ret = btrfs_init_inode_security(trans, inode, dir, NULL);
+	if (ret)
+		goto out;
+
+	ret = btrfs_update_inode(trans, root, inode);
+	if (ret)
+		goto out;
+
+	inode->i_fop = &btrfs_file_operations;
+	inode->i_op = &btrfs_file_inode_operations;
+
+	inode->i_mapping->a_ops = &btrfs_aops;
+	inode->i_mapping->backing_dev_info = &root->fs_info->bdi;
+	BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
+
+	ret = btrfs_orphan_add(trans, inode);
+	if (ret)
+		goto out;
+
+	d_tmpfile(dentry, inode);
+	mark_inode_dirty(inode);
+
+out:
+	btrfs_end_transaction(trans, root);
+	if (ret)
+		iput(inode);
+	btrfs_balance_delayed_items(root);
+	btrfs_btree_balance_dirty(root);
+
+	return ret;
+}
+
 static const struct inode_operations btrfs_dir_inode_operations = {
 	.getattr	= btrfs_getattr,
 	.lookup		= btrfs_lookup,
@@ -8877,6 +8956,7 @@ static const struct inode_operations btrfs_dir_inode_operations = {
 	.permission	= btrfs_permission,
 	.get_acl	= btrfs_get_acl,
 	.update_time	= btrfs_update_time,
+	.tmpfile        = btrfs_tmpfile,
 };
 static const struct inode_operations btrfs_dir_ro_inode_operations = {
 	.lookup		= btrfs_lookup,
-- 
1.9.1

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[David Sterba]

On Tue, Apr 01, 2014 at 11:53:19PM +0100, Filipe David Borba Manana wrote:
> This implements the tmpfile callback of struct inode_operations, introduced
> in the linux kernel 3.11 [1], and implemented already by some filesystems.

Nice!

Btw, would be good to mention 'O_TMPFILE' at lest in the changelog.

> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
Reviewed-by: David Sterba <dsterba@suse.cz>

 CC: linuxpatches@star.c10r.facebook.com

how many likes so far?

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[Chris Mason]

On 04/04/2014 09:59 AM, David Sterba wrote:
> On Tue, Apr 01, 2014 at 11:53:19PM +0100, Filipe David Borba Manana wrote:
>> This implements the tmpfile callback of struct inode_operations, introduced
>> in the linux kernel 3.11 [1], and implemented already by some filesystems.
>
> Nice!
>
> Btw, would be good to mention 'O_TMPFILE' at lest in the changelog.
>
>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
> Reviewed-by: David Sterba <dsterba@suse.cz>
>
>   CC: linuxpatches@star.c10r.facebook.com
>
> how many likes so far?

He got a Nice from me ;)  That's for when a like alone won't do.

It doesn't look like xfstests has an O_TMPFILE test?

-chris

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[Filipe David Manana]

On Fri, Apr 4, 2014 at 3:12 PM, Chris Mason <clm@fb.com> wrote:
>
>
> On 04/04/2014 09:59 AM, David Sterba wrote:
>>
>> On Tue, Apr 01, 2014 at 11:53:19PM +0100, Filipe David Borba Manana wrote:
>>>
>>> This implements the tmpfile callback of struct inode_operations,
>>> introduced
>>> in the linux kernel 3.11 [1], and implemented already by some
>>> filesystems.
>>
>>
>> Nice!
>>
>> Btw, would be good to mention 'O_TMPFILE' at lest in the changelog.
>>
>>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>>
>> Reviewed-by: David Sterba <dsterba@suse.cz>
>>
>>   CC: linuxpatches@star.c10r.facebook.com
>>
>> how many likes so far?
>
>
> He got a Nice from me ;)  That's for when a like alone won't do.
>
> It doesn't look like xfstests has an O_TMPFILE test?

It has now, if you look at Dave's latest xfstests update e-mail he
sent earlier today.
For reference, I used this for testing:
https://friendpaste.com/4n46iUoo4ZdqTAdkPntqYe (plus scrub, send,
btrfsck, etc)

>
> -chris
>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[Chandan Rajendra]

On Tuesday 01 Apr 2014 11:53:19 PM Filipe David Borba Manana wrote:
> +static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
> +{
> +	struct btrfs_trans_handle *trans;
> +	struct btrfs_root *root = BTRFS_I(dir)->root;
> +	struct inode *inode = NULL;
> +	u64 objectid;
> +	u64 index;
> +	int ret = 0;
> +
> +	/*
> +	 * 2 for inode item and ref
> +	 * 2 for dir items
> +	 * 1 for xattr if selinux is on
> +	 */
> +	trans = btrfs_start_transaction(root, 5);
> +	if (IS_ERR(trans))
> +		return PTR_ERR(trans);
> +

Hello,

Any particular reason to reserve space for 5 items? For the O_TMPFILE
case we seem to allocate and use just the one inode item and none of
the associated 'inode ref', 'dir item' and 'dir index item' since
there is no directory entry associated with the file. I am not
sure about the xattr item though.

Thanks,
chandan.

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[Filipe David Manana]

On Wed, Apr 16, 2014 at 10:00 AM, Chandan Rajendra
<chandan@linux.vnet.ibm.com> wrote:
> On Tuesday 01 Apr 2014 11:53:19 PM Filipe David Borba Manana wrote:
>> +static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
>> +{
>> +     struct btrfs_trans_handle *trans;
>> +     struct btrfs_root *root = BTRFS_I(dir)->root;
>> +     struct inode *inode = NULL;
>> +     u64 objectid;
>> +     u64 index;
>> +     int ret = 0;
>> +
>> +     /*
>> +      * 2 for inode item and ref
>> +      * 2 for dir items
>> +      * 1 for xattr if selinux is on
>> +      */
>> +     trans = btrfs_start_transaction(root, 5);
>> +     if (IS_ERR(trans))
>> +             return PTR_ERR(trans);
>> +
>
> Hello,
>
> Any particular reason to reserve space for 5 items? For the O_TMPFILE
> case we seem to allocate and use just the one inode item and none of
> the associated 'inode ref', 'dir item' and 'dir index item' since
> there is no directory entry associated with the file. I am not
> sure about the xattr item though.

Correct, not needed for directory entries.
The xattr is needed for the case where an acl is inherited. And 5
units are required for orphan insertion (see comment on top of
btrfs_orphan_add).
I'll update the comment.

Thanks

>
> Thanks,
> chandan.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[Christoph Hellwig]

On Wed, Apr 16, 2014 at 11:36:23AM +0100, Filipe David Manana wrote:
> The xattr is needed for the case where an acl is inherited. And 5
> units are required for orphan insertion (see comment on top of
> btrfs_orphan_add).
> I'll update the comment.

I don't think think a tmpfile should inherit any ACL, as it does not
have a parent.  Anyway, we're currently having a discussion on that on
various lists including linux-fsdevel.

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[Filipe David Manana]

On Wed, Apr 16, 2014 at 12:09 PM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Apr 16, 2014 at 11:36:23AM +0100, Filipe David Manana wrote:
>> The xattr is needed for the case where an acl is inherited. And 5
>> units are required for orphan insertion (see comment on top of
>> btrfs_orphan_add).
>> I'll update the comment.
>
> I don't think think a tmpfile should inherit any ACL, as it does not
> have a parent.  Anyway, we're currently having a discussion on that on
> various lists including linux-fsdevel.

Interesting Christoph.
I was following the ext4 implementation initially.

So it seems the question is still open, and none of the following
alternatives is decided yet (unless I missed something in the thread
at fsdevel):

1) inherit acl from directory passed to the tmpfile handler;
2) inherit acl at link time from directory we're linking to;
3) don't inherit acls

Thanks for the heads up.

>



-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."

Re: [PATCH] Btrfs: implement inode_operations callback tmpfile

[Christoph Hellwig]

On Wed, Apr 16, 2014 at 12:25:07PM +0100, Filipe David Manana wrote:
> Interesting Christoph.
> I was following the ext4 implementation initially.
> 
> So it seems the question is still open, and none of the following
> alternatives is decided yet (unless I missed something in the thread
> at fsdevel):
> 
> 1) inherit acl from directory passed to the tmpfile handler;
> 2) inherit acl at link time from directory we're linking to;
> 3) don't inherit acls
> 
> Thanks for the heads up.

I'll make sure we'll have a testcase in xfstests to cover whatever
behavior we decide on to make sure all filesystems behave the same way.

[PATCH v2] Btrfs: implement inode_operations callback tmpfile

[Filipe David Borba Manana]

This implements the tmpfile callback of struct inode_operations, introduced
in the linux kernel 3.11, and implemented already by some filesystems. This
callback is invoked by the VFS when the flag O_TMPFILE is passed to the open
system call.

Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---

V2: Updated change log and comment about how many metadata units are needed
    for the transaction.
    Left the ACL inheritance in the callback (like ext4 does) since the thread
    in linux-fsdevel seems to have ended with the conclusion that this is the
    right behaviour (as Andreas Gruenbacher says).

 fs/btrfs/inode.c | 118 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 98 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 0c0bb45..b5397db 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5549,6 +5549,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	struct btrfs_inode_ref *ref;
 	struct btrfs_key key[2];
 	u32 sizes[2];
+	int nitems = name ? 2 : 1;
 	unsigned long ptr;
 	int ret;
 
@@ -5568,7 +5569,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	 */
 	inode->i_ino = objectid;
 
-	if (dir) {
+	if (dir && name) {
 		trace_btrfs_inode_request(dir);
 
 		ret = btrfs_set_inode_index(dir, index);
@@ -5577,6 +5578,8 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 			iput(inode);
 			return ERR_PTR(ret);
 		}
+	} else if (dir) {
+		*index = 0;
 	}
 	/*
 	 * index_cnt is ignored for everything but a dir,
@@ -5601,21 +5604,24 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 	btrfs_set_key_type(&key[0], BTRFS_INODE_ITEM_KEY);
 	key[0].offset = 0;
 
-	/*
-	 * Start new inodes with an inode_ref. This is slightly more
-	 * efficient for small numbers of hard links since they will
-	 * be packed into one item. Extended refs will kick in if we
-	 * add more hard links than can fit in the ref item.
-	 */
-	key[1].objectid = objectid;
-	btrfs_set_key_type(&key[1], BTRFS_INODE_REF_KEY);
-	key[1].offset = ref_objectid;
-
 	sizes[0] = sizeof(struct btrfs_inode_item);
-	sizes[1] = name_len + sizeof(*ref);
+
+	if (name) {
+		/*
+		 * Start new inodes with an inode_ref. This is slightly more
+		 * efficient for small numbers of hard links since they will
+		 * be packed into one item. Extended refs will kick in if we
+		 * add more hard links than can fit in the ref item.
+		 */
+		key[1].objectid = objectid;
+		btrfs_set_key_type(&key[1], BTRFS_INODE_REF_KEY);
+		key[1].offset = ref_objectid;
+
+		sizes[1] = name_len + sizeof(*ref);
+	}
 
 	path->leave_spinning = 1;
-	ret = btrfs_insert_empty_items(trans, root, path, key, sizes, 2);
+	ret = btrfs_insert_empty_items(trans, root, path, key, sizes, nitems);
 	if (ret != 0)
 		goto fail;
 
@@ -5628,12 +5634,14 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 			     sizeof(*inode_item));
 	fill_inode_item(trans, path->nodes[0], inode_item, inode);
 
-	ref = btrfs_item_ptr(path->nodes[0], path->slots[0] + 1,
-			     struct btrfs_inode_ref);
-	btrfs_set_inode_ref_name_len(path->nodes[0], ref, name_len);
-	btrfs_set_inode_ref_index(path->nodes[0], ref, *index);
-	ptr = (unsigned long)(ref + 1);
-	write_extent_buffer(path->nodes[0], name, ptr, name_len);
+	if (name) {
+		ref = btrfs_item_ptr(path->nodes[0], path->slots[0] + 1,
+				     struct btrfs_inode_ref);
+		btrfs_set_inode_ref_name_len(path->nodes[0], ref, name_len);
+		btrfs_set_inode_ref_index(path->nodes[0], ref, *index);
+		ptr = (unsigned long)(ref + 1);
+		write_extent_buffer(path->nodes[0], name, ptr, name_len);
+	}
 
 	btrfs_mark_buffer_dirty(path->nodes[0]);
 	btrfs_free_path(path);
@@ -5669,7 +5677,7 @@ static struct inode *btrfs_new_inode(struct btrfs_trans_handle *trans,
 
 	return inode;
 fail:
-	if (dir)
+	if (dir && name)
 		BTRFS_I(dir)->index_cnt--;
 	btrfs_free_path(path);
 	iput(inode);
@@ -5954,6 +5962,15 @@ static int btrfs_link(struct dentry *old_dentry, struct inode *dir,
 		err = btrfs_update_inode(trans, root, inode);
 		if (err)
 			goto fail;
+		if (inode->i_nlink == 1) {
+			/*
+			 * If new hard link count is 1, it's a file created
+			 * with open(2) O_TMPFILE flag.
+			 */
+			err = btrfs_orphan_del(trans, inode);
+			if (err)
+				goto fail;
+		}
 		d_instantiate(dentry, inode);
 		btrfs_log_new_name(trans, inode, NULL, parent);
 	}
@@ -8886,6 +8903,66 @@ static int btrfs_permission(struct inode *inode, int mask)
 	return generic_permission(inode, mask);
 }
 
+static int btrfs_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
+{
+	struct btrfs_trans_handle *trans;
+	struct btrfs_root *root = BTRFS_I(dir)->root;
+	struct inode *inode = NULL;
+	u64 objectid;
+	u64 index;
+	int ret = 0;
+
+	/*
+	 * 5 units required for adding orphan entry
+	 */
+	trans = btrfs_start_transaction(root, 5);
+	if (IS_ERR(trans))
+		return PTR_ERR(trans);
+
+	ret = btrfs_find_free_ino(root, &objectid);
+	if (ret)
+		goto out;
+
+	inode = btrfs_new_inode(trans, root, dir, NULL, 0,
+				btrfs_ino(dir), objectid, mode, &index);
+	if (IS_ERR(inode)) {
+		ret = PTR_ERR(inode);
+		inode = NULL;
+		goto out;
+	}
+
+	ret = btrfs_init_inode_security(trans, inode, dir, NULL);
+	if (ret)
+		goto out;
+
+	ret = btrfs_update_inode(trans, root, inode);
+	if (ret)
+		goto out;
+
+	inode->i_fop = &btrfs_file_operations;
+	inode->i_op = &btrfs_file_inode_operations;
+
+	inode->i_mapping->a_ops = &btrfs_aops;
+	inode->i_mapping->backing_dev_info = &root->fs_info->bdi;
+	BTRFS_I(inode)->io_tree.ops = &btrfs_extent_io_ops;
+
+	ret = btrfs_orphan_add(trans, inode);
+	if (ret)
+		goto out;
+
+	d_tmpfile(dentry, inode);
+	mark_inode_dirty(inode);
+
+out:
+	btrfs_end_transaction(trans, root);
+	if (ret)
+		iput(inode);
+	btrfs_balance_delayed_items(root);
+	btrfs_btree_balance_dirty(root);
+
+	return ret;
+}
+
 static const struct inode_operations btrfs_dir_inode_operations = {
 	.getattr	= btrfs_getattr,
 	.lookup		= btrfs_lookup,
@@ -8905,6 +8982,7 @@ static const struct inode_operations btrfs_dir_inode_operations = {
 	.permission	= btrfs_permission,
 	.get_acl	= btrfs_get_acl,
 	.update_time	= btrfs_update_time,
+	.tmpfile        = btrfs_tmpfile,
 };
 static const struct inode_operations btrfs_dir_ro_inode_operations = {
 	.lookup		= btrfs_lookup,
-- 
1.9.1