Re: [PATCH v2 5/6] Btrfs: use flags instead of the bool variants in delayed node

[Miao Xie <miaox@cn.fujitsu.com>]

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_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.
> 
> 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
>