| Message ID | 1396392799-9858-1-git-send-email-fdmanana@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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? -- 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
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 -- 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
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 >
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. -- 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
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
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. -- 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
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. >
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. -- 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
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,
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(-)