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
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 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 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
[PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node
Signed-off-by: Miao Xie mi...@cn.fujitsu.com
---
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(delayed_node);
return -ENOENT;
@@ -1772,7 +1773,7 @@ int
