[PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node
Signed-off-by: Miao Xie --- Changelog v1 -> v2: - None. --- fs/btrfs/delayed-inode.c | 33 + fs/btrfs/delayed-inode.h | 6 -- 2 files changed, 21 insertions(+), 18 deletions(-) diff --git a/fs/btrfs/delayed-inode.c b/fs/btrfs/delayed-inode.c index 7d55443..979db56 100644 --- a/fs/btrfs/delayed-inode.c +++ b/fs/btrfs/delayed-inode.c @@ -55,8 +55,7 @@ static inline void btrfs_init_delayed_node( delayed_node->inode_id = inode_id; atomic_set(&delayed_node->refs, 0); delayed_node->count = 0; - delayed_node->in_list = 0; - delayed_node->inode_dirty = 0; + delayed_node->flags = 0; delayed_node->ins_root = RB_ROOT; delayed_node->del_root = RB_ROOT; mutex_init(&delayed_node->mutex); @@ -172,7 +171,7 @@ static void btrfs_queue_delayed_node(struct btrfs_delayed_root *root, int mod) { spin_lock(&root->lock); - if (node->in_list) { + if (test_bit(BTRFS_DELAYED_NODE_IN_LIST, &node->flags)) { if (!list_empty(&node->p_list)) list_move_tail(&node->p_list, &root->prepare_list); else if (mod) @@ -182,7 +181,7 @@ static void btrfs_queue_delayed_node(struct btrfs_delayed_root *root, list_add_tail(&node->p_list, &root->prepare_list); atomic_inc(&node->refs);/* inserted into list */ root->nodes++; - node->in_list = 1; + set_bit(BTRFS_DELAYED_NODE_IN_LIST, &node->flags); } spin_unlock(&root->lock); } @@ -192,13 +191,13 @@ static void btrfs_dequeue_delayed_node(struct btrfs_delayed_root *root, struct btrfs_delayed_node *node) { spin_lock(&root->lock); - if (node->in_list) { + if (test_bit(BTRFS_DELAYED_NODE_IN_LIST, &node->flags)) { root->nodes--; atomic_dec(&node->refs);/* not in the list */ list_del_init(&node->n_list); if (!list_empty(&node->p_list)) list_del_init(&node->p_list); - node->in_list = 0; + clear_bit(BTRFS_DELAYED_NODE_IN_LIST, &node->flags); } spin_unlock(&root->lock); } @@ -231,7 +230,8 @@ static struct btrfs_delayed_node *btrfs_next_delayed_node( delayed_root = node->root->fs_info->delayed_root; spin_lock(&delayed_root->lock); - if (!node->in_list) { /* not in the list */ + if (!test_bit(BTRFS_DELAYED_NODE_IN_LIST, &node->flags)) { + /* not in the list */ if (list_empty(&delayed_root->node_list)) goto out; p = delayed_root->node_list.next; @@ -1004,9 +1004,10 @@ static void btrfs_release_delayed_inode(struct btrfs_delayed_node *delayed_node) { struct btrfs_delayed_root *delayed_root; - if (delayed_node && delayed_node->inode_dirty) { + if (delayed_node && + test_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &delayed_node->flags)) { BUG_ON(!delayed_node->root); - delayed_node->inode_dirty = 0; + clear_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &delayed_node->flags); delayed_node->count--; delayed_root = delayed_node->root->fs_info->delayed_root; @@ -1059,7 +1060,7 @@ static inline int btrfs_update_delayed_inode(struct btrfs_trans_handle *trans, int ret; mutex_lock(&node->mutex); - if (!node->inode_dirty) { + if (!test_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &node->flags)) { mutex_unlock(&node->mutex); return 0; } @@ -1203,7 +1204,7 @@ int btrfs_commit_inode_delayed_inode(struct inode *inode) return 0; mutex_lock(&delayed_node->mutex); - if (!delayed_node->inode_dirty) { + if (!test_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &delayed_node->flags)) { mutex_unlock(&delayed_node->mutex); btrfs_release_delayed_node(delayed_node); return 0; @@ -1227,7 +1228,7 @@ int btrfs_commit_inode_delayed_inode(struct inode *inode) trans->block_rsv = &delayed_node->root->fs_info->delayed_block_rsv; mutex_lock(&delayed_node->mutex); - if (delayed_node->inode_dirty) + if (test_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &delayed_node->flags)) ret = __btrfs_update_delayed_inode(trans, delayed_node->root, path, delayed_node); else @@ -1721,7 +1722,7 @@ int btrfs_fill_inode(struct inode *inode, u32 *rdev) return -ENOENT; mutex_lock(&delayed_node->mutex); - if (!delayed_node->inode_dirty) { + if (!test_bit(BTRFS_DELAYED_NODE_INODE_DIRTY, &delayed_node->flags)) { mutex_unlock(&delayed_node->mutex); btrfs_release_delayed_node
Re: [PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node
On Thu, Dec 26, 2013 at 01:07:05PM +0800, Miao Xie wrote: > +#define BTRFS_DELAYED_NODE_IN_LIST 0 > +#define BTRFS_DELAYED_NODE_INODE_DIRTY 1 > + > struct btrfs_delayed_node { > u64 inode_id; > u64 bytes_reserved; > @@ -65,8 +68,7 @@ struct btrfs_delayed_node { > struct btrfs_inode_item inode_item; > atomic_t refs; > u64 index_cnt; > - bool in_list; > - bool inode_dirty; > + unsigned long flags; > int count; > }; What's the reason to do that? Replacing 2 bools with a bitfield does not seem justified, not from saving memory, nor from a performance gain side. Also some of the bit operations imply the lock instruction prefix so this affects the surrounding items as well. I don't think this is needed, unless you have further plans with the flags item. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node
On Thu, 2 Jan 2014 18:49:55 +0100, David Sterba wrote: > On Thu, Dec 26, 2013 at 01:07:05PM +0800, Miao Xie wrote: >> +#define BTRFS_DELAYED_NODE_IN_LIST 0 >> +#define BTRFS_DELAYED_NODE_INODE_DIRTY 1 >> + >> struct btrfs_delayed_node { >> u64 inode_id; >> u64 bytes_reserved; >> @@ -65,8 +68,7 @@ struct btrfs_delayed_node { >> struct btrfs_inode_item inode_item; >> atomic_t refs; >> u64 index_cnt; >> -bool in_list; >> -bool inode_dirty; >> +unsigned long flags; >> int count; >> }; > > What's the reason to do that? Replacing 2 bools with a bitfield > does not seem justified, not from saving memory, nor from a performance > gain side. Also some of the bit operations imply the lock instruction > prefix so this affects the surrounding items as well. > > I don't think this is needed, unless you have further plans with the > flags item. Yes, I introduced a flag in the next patch. And we can not common bit operation here because we hold different locks when we change those bits, we have to use the bit operations which implies the lock instruction. Thanks Miao -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node
On Fri, Jan 03, 2014 at 05:27:51PM +0800, Miao Xie wrote: > On Thu, 2 Jan 2014 18:49:55 +0100, David Sterba wrote: > > On Thu, Dec 26, 2013 at 01:07:05PM +0800, Miao Xie wrote: > >> +#define BTRFS_DELAYED_NODE_IN_LIST0 > >> +#define BTRFS_DELAYED_NODE_INODE_DIRTY1 > >> + > >> struct btrfs_delayed_node { > >>u64 inode_id; > >>u64 bytes_reserved; > >> @@ -65,8 +68,7 @@ struct btrfs_delayed_node { > >>struct btrfs_inode_item inode_item; > >>atomic_t refs; > >>u64 index_cnt; > >> - bool in_list; > >> - bool inode_dirty; > >> + unsigned long flags; > >>int count; > >> }; > > > > What's the reason to do that? Replacing 2 bools with a bitfield > > does not seem justified, not from saving memory, nor from a performance > > gain side. Also some of the bit operations imply the lock instruction > > prefix so this affects the surrounding items as well. > > > > I don't think this is needed, unless you have further plans with the > > flags item. > > Yes, I introduced a flag in the next patch. That's still 3 bool flags that are quite independent and consume less than the unsigned long anyway. Also the bool flags are something that compiler understands and can use during optimizations unlike the obfuscated bit access. I don't mind using bitfields, but it imo starts to make sense to use them when there are more than a few, like BTRFS_INODE_* or EXTENT_BUFFER_*. The point of my objections is to establish good coding patterns to follow. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node
On Fri, 3 Jan 2014 19:36:10 +0100, David Sterba wrote: > On Fri, Jan 03, 2014 at 05:27:51PM +0800, Miao Xie wrote: >> On Thu, 2 Jan 2014 18:49:55 +0100, David Sterba wrote: >>> On Thu, Dec 26, 2013 at 01:07:05PM +0800, Miao Xie wrote: +#define BTRFS_DELAYED_NODE_IN_LIST0 +#define BTRFS_DELAYED_NODE_INODE_DIRTY1 + struct btrfs_delayed_node { u64 inode_id; u64 bytes_reserved; @@ -65,8 +68,7 @@ struct btrfs_delayed_node { struct btrfs_inode_item inode_item; atomic_t refs; u64 index_cnt; - bool in_list; - bool inode_dirty; + unsigned long flags; int count; }; >>> >>> What's the reason to do that? Replacing 2 bools with a bitfield >>> does not seem justified, not from saving memory, nor from a performance >>> gain side. Also some of the bit operations imply the lock instruction >>> prefix so this affects the surrounding items as well. >>> >>> I don't think this is needed, unless you have further plans with the >>> flags item. >> >> Yes, I introduced a flag in the next patch. > > That's still 3 bool flags that are quite independent and consume less > than the unsigned long anyway. Also the bool flags are something that > compiler understands and can use during optimizations unlike the > obfuscated bit access. I made a mistake at the beginning, I though the size of boolean data type in the kernel was 4 bytes because I saw it was implemented by the enumeration type several years ago. But it has been changed for many years. So you are right. But I read a discuss about the use of boolean type, some developers suggested us to use bitfields instead of bool, because the bitfields can work better, and they are more flexible, less misuse than bool. https://lkml.org/lkml/2013/9/1/154 Furthermore, we may introduce more flags in the future though I have no plan now. So this patch makes sense I think. Thanks Miao > I don't mind using bitfields, but it imo starts to make sense to use > them when there are more than a few, like BTRFS_INODE_* or > EXTENT_BUFFER_*. The point of my objections is to establish good coding > patterns to follow. > > david > -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node
On Tue, Jan 07, 2014 at 11:59:18AM +0800, Miao Xie wrote: > But I read a discuss about the use of boolean type, some developers suggested > us to use bitfields instead of bool, because the bitfields can work better, > and they are more flexible, less misuse than bool. > https://lkml.org/lkml/2013/9/1/154 I was searching for pros/cons of the bools but haven't found this one nor anything useful, though not all of the advantages of bitfields apply in our case. I've compared the generated assembly with just this patch, and the difference is effectively only the lock prefix for set/clear, the read side has no significant penalty: old: mov0x128(%rbx),%esi new: mov0x120(%rbx),%rax test $0x1,%al old set: movb $0x1,0x120(%rbx) new: lock orb $0x1,0x120(%rbx) The delayed_node structure is relatively short lived, is reused frequently and I haven't seen any contention points where the lock prefix would hurt. So, ok to merge it. -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html