[PATCH 1/2] btrfs: remove unused sectorsize member

[PATCH 1/2] btrfs: remove unused sectorsize member

[Nikolay Borisov]

The sectorsize member of btrfs_block_group_cache is unused. So remove it, this
reduces the number of holes in the struct.

With patch:
/* size: 856, cachelines: 14, members: 40 */
/* sum members: 837, holes: 4, sum holes: 19 */
/* bit holes: 1, sum bit holes: 29 bits */
/* last cacheline: 24 bytes */

Without patch:
/* size: 864, cachelines: 14, members: 41 */
/* sum members: 841, holes: 5, sum holes: 23 */
/* bit holes: 1, sum bit holes: 29 bits */
/* last cacheline: 32 bytes */

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h       | 1 -
 fs/btrfs/extent-tree.c | 1 -
 2 files changed, 2 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a75a23f9d68e..cdd3775e930b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -559,7 +559,6 @@ struct btrfs_block_group_cache {
 	u64 bytes_super;
 	u64 flags;
 	u64 cache_generation;
-	u32 sectorsize;
 
 	/*
 	 * If the free space extent count exceeds this number, convert the block
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a08a743a8e09..2a0d300c7d1a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -9904,7 +9904,6 @@ btrfs_create_block_group_cache(struct btrfs_fs_info *fs_info,
 	cache->key.offset = size;
 	cache->key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
 
-	cache->sectorsize = fs_info->sectorsize;
 	cache->fs_info = fs_info;
 	cache->full_stripe_len = btrfs_full_stripe_len(fs_info,
 						       &fs_info->mapping_tree,
-- 
2.7.4

[PATCH 2/2] btrfs: Optimise layout of btrfs_block_group_cache

[Nikolay Borisov]

With this patch applied pahole stats look like:

/* size: 840, cachelines: 14, members: 40 */
/* sum members: 833, holes: 1, sum holes: 7 */
/* bit holes: 1, sum bit holes: 28 bits */
/* last cacheline: 8 bytes */

No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/ctree.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index cdd3775e930b..bdd06bbeb9aa 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -586,6 +586,11 @@ struct btrfs_block_group_cache {
 	unsigned int iref:1;
 	unsigned int has_caching_ctl:1;
 	unsigned int removed:1;
+	/*
+	 * Does the block group need to be added to the free space tree?
+	 * Protected by free_space_lock.
+	 */
+	unsigned int needs_free_space:1;
 
 	int disk_cache_state;
 
@@ -608,6 +613,8 @@ struct btrfs_block_group_cache {
 	/* usage count */
 	atomic_t count;
 
+	atomic_t trimming;
+
 	/* List of struct btrfs_free_clusters for this block group.
 	 * Today it will only have one thing on it, but that may change
 	 */
@@ -619,8 +626,6 @@ struct btrfs_block_group_cache {
 	/* For read-only block groups */
 	struct list_head ro_list;
 
-	atomic_t trimming;
-
 	/* For dirty block groups */
 	struct list_head dirty_list;
 	struct list_head io_list;
@@ -651,11 +656,6 @@ struct btrfs_block_group_cache {
 	/* Lock for free space tree operations. */
 	struct mutex free_space_lock;
 
-	/*
-	 * Does the block group need to be added to the free space tree?
-	 * Protected by free_space_lock.
-	 */
-	int needs_free_space;
 
 	/* Record locked full stripes for RAID5/6 block group */
 	struct btrfs_full_stripe_locks_tree full_stripe_locks_root;
-- 
2.7.4

Re: [PATCH 2/2] btrfs: Optimise layout of btrfs_block_group_cache

[Nikolay Borisov]

On 26.06.2017 17:42, Nikolay Borisov wrote:
> With this patch applied pahole stats look like:
> 
> /* size: 840, cachelines: 14, members: 40 */
> /* sum members: 833, holes: 1, sum holes: 7 */
> /* bit holes: 1, sum bit holes: 28 bits */
> /* last cacheline: 8 bytes */
> 
> No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/ctree.h | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index cdd3775e930b..bdd06bbeb9aa 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -586,6 +586,11 @@ struct btrfs_block_group_cache {
>  	unsigned int iref:1;
>  	unsigned int has_caching_ctl:1;
>  	unsigned int removed:1;
> +	/*
> +	 * Does the block group need to be added to the free space tree?
> +	 * Protected by free_space_lock.
> +	 */
> +	unsigned int needs_free_space:1;
Upon closer inspection of memory-barriers.txt I'm not confident in this
change. This puts fields protected by different locks in the same
bitfield which can lead to corrupted values.

>  
>  	int disk_cache_state;
>  
> @@ -608,6 +613,8 @@ struct btrfs_block_group_cache {
>  	/* usage count */
>  	atomic_t count;
>  
> +	atomic_t trimming

This one will likely eliminated 1 hole in the struct so I might end up
sending v2 of this patch.

> +
>  	/* List of struct btrfs_free_clusters for this block group.
>  	 * Today it will only have one thing on it, but that may change
>  	 */
> @@ -619,8 +626,6 @@ struct btrfs_block_group_cache {
>  	/* For read-only block groups */
>  	struct list_head ro_list;
>  
> -	atomic_t trimming;
> -
>  	/* For dirty block groups */
>  	struct list_head dirty_list;
>  	struct list_head io_list;
> @@ -651,11 +656,6 @@ struct btrfs_block_group_cache {
>  	/* Lock for free space tree operations. */
>  	struct mutex free_space_lock;
>  
> -	/*
> -	 * Does the block group need to be added to the free space tree?
> -	 * Protected by free_space_lock.
> -	 */
> -	int needs_free_space;
>  
>  	/* Record locked full stripes for RAID5/6 block group */
>  	struct btrfs_full_stripe_locks_tree full_stripe_locks_root;
> 

Re: [PATCH 2/2] btrfs: Optimise layout of btrfs_block_group_cache

[David Sterba]

On Mon, Jun 26, 2017 at 06:30:34PM +0300, Nikolay Borisov wrote:
> 
> 
> On 26.06.2017 17:42, Nikolay Borisov wrote:
> > With this patch applied pahole stats look like:
> > 
> > /* size: 840, cachelines: 14, members: 40 */
> > /* sum members: 833, holes: 1, sum holes: 7 */
> > /* bit holes: 1, sum bit holes: 28 bits */
> > /* last cacheline: 8 bytes */
> > 
> > No functional changes.
> > 
> > Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> > ---
> >  fs/btrfs/ctree.h | 14 +++++++-------
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> > index cdd3775e930b..bdd06bbeb9aa 100644
> > --- a/fs/btrfs/ctree.h
> > +++ b/fs/btrfs/ctree.h
> > @@ -586,6 +586,11 @@ struct btrfs_block_group_cache {
> >  	unsigned int iref:1;
> >  	unsigned int has_caching_ctl:1;
> >  	unsigned int removed:1;
> > +	/*
> > +	 * Does the block group need to be added to the free space tree?
> > +	 * Protected by free_space_lock.
> > +	 */
> > +	unsigned int needs_free_space:1;
> Upon closer inspection of memory-barriers.txt I'm not confident in this
> change. This puts fields protected by different locks in the same
> bitfield which can lead to corrupted values.

Alternatively, you can switch the flags to unsigned long and access them
using set_bit/test_bit, they're atomic regarding updates.

Re: [PATCH 1/2] btrfs: remove unused sectorsize member

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2325 bytes --]

Hi Nikolay,

[auto build test ERROR on v4.12-rc6]
[also build test ERROR on next-20170627]
[cannot apply to btrfs/next]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikolay-Borisov/btrfs-remove-unused-sectorsize-member/20170627-183835
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 4.9.0
reproduce:
        wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   fs/btrfs/tests/btrfs-tests.c: In function 'btrfs_alloc_dummy_block_group':
>> fs/btrfs/tests/btrfs-tests.c:214:7: error: 'struct btrfs_block_group_cache' has no member named 'sectorsize'
     cache->sectorsize = fs_info->sectorsize;
          ^
--
   fs/btrfs/tests/free-space-tree-tests.c: In function '__check_free_space_extents':
>> fs/btrfs/tests/free-space-tree-tests.c:84:20: error: 'struct btrfs_block_group_cache' has no member named 'sectorsize'
        offset += cache->sectorsize;
                       ^

vim +214 fs/btrfs/tests/btrfs-tests.c

7c55ee0c Omar Sandoval 2015-09-29  208  		return NULL;
7c55ee0c Omar Sandoval 2015-09-29  209  	}
7c55ee0c Omar Sandoval 2015-09-29  210  
7c55ee0c Omar Sandoval 2015-09-29  211  	cache->key.objectid = 0;
7c55ee0c Omar Sandoval 2015-09-29  212  	cache->key.offset = length;
7c55ee0c Omar Sandoval 2015-09-29  213  	cache->key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
da17066c Jeff Mahoney  2016-06-15 @214  	cache->sectorsize = fs_info->sectorsize;
da17066c Jeff Mahoney  2016-06-15  215  	cache->full_stripe_len = fs_info->sectorsize;
da17066c Jeff Mahoney  2016-06-15  216  	cache->fs_info = fs_info;
7c55ee0c Omar Sandoval 2015-09-29  217  

:::::: The code at line 214 was first introduced by commit
:::::: da17066c40472c2d6a1aab7bb0090c3d285531c9 btrfs: pull node/sector/stripe sizes out of root and into fs_info

:::::: TO: Jeff Mahoney <jeffm@suse.com>
:::::: CC: David Sterba <dsterba@suse.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50281 bytes --]

[PATCH v2] btrfs: remove unused sectorsize member

[Nikolay Borisov]

The sectorsize member of btrfs_block_group_cache is unused. So remove it, this
reduces the number of holes in the struct.

With patch:
/* size: 856, cachelines: 14, members: 40 */
/* sum members: 837, holes: 4, sum holes: 19 */
/* bit holes: 1, sum bit holes: 29 bits */
/* last cacheline: 24 bytes */

Without patch:
/* size: 864, cachelines: 14, members: 41 */
/* sum members: 841, holes: 5, sum holes: 23 */
/* bit holes: 1, sum bit holes: 29 bits */
/* last cacheline: 32 bytes */
---

Changes since v1: 
 * Missed the usage in btrfs-tests.c

 fs/btrfs/ctree.h             | 1 -
 fs/btrfs/extent-tree.c       | 1 -
 fs/btrfs/tests/btrfs-tests.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a75a23f9d68e..cdd3775e930b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -559,7 +559,6 @@ struct btrfs_block_group_cache {
 	u64 bytes_super;
 	u64 flags;
 	u64 cache_generation;
-	u32 sectorsize;
 
 	/*
 	 * If the free space extent count exceeds this number, convert the block
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a08a743a8e09..2a0d300c7d1a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -9904,7 +9904,6 @@ btrfs_create_block_group_cache(struct btrfs_fs_info *fs_info,
 	cache->key.offset = size;
 	cache->key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
 
-	cache->sectorsize = fs_info->sectorsize;
 	cache->fs_info = fs_info;
 	cache->full_stripe_len = btrfs_full_stripe_len(fs_info,
 						       &fs_info->mapping_tree,
diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index b18ab8f327a5..d3f25376a0f8 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -211,7 +211,6 @@ btrfs_alloc_dummy_block_group(struct btrfs_fs_info *fs_info,
 	cache->key.objectid = 0;
 	cache->key.offset = length;
 	cache->key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
-	cache->sectorsize = fs_info->sectorsize;
 	cache->full_stripe_len = fs_info->sectorsize;
 	cache->fs_info = fs_info;
 
-- 
2.7.4

Re: [PATCH v2] btrfs: remove unused sectorsize member

[David Sterba]

On Tue, Jun 27, 2017 at 04:25:52PM +0300, Nikolay Borisov wrote:
> The sectorsize member of btrfs_block_group_cache is unused. So remove it, this
> reduces the number of holes in the struct.
> 
> With patch:
> /* size: 856, cachelines: 14, members: 40 */
> /* sum members: 837, holes: 4, sum holes: 19 */
> /* bit holes: 1, sum bit holes: 29 bits */
> /* last cacheline: 24 bytes */
> 
> Without patch:
> /* size: 864, cachelines: 14, members: 41 */
> /* sum members: 841, holes: 5, sum holes: 23 */
> /* bit holes: 1, sum bit holes: 29 bits */
> /* last cacheline: 32 bytes */

Signed-off-by: ...

otherwise ok,

Reviewed-by: David Sterba <dsterba@suse.com>

[PATCH v3] btrfs: remove unused sectorsize member

[Nikolay Borisov]

The sectorsize member of btrfs_block_group_cache is unused. So remove it, this
reduces the number of holes in the struct.

With patch:
/* size: 856, cachelines: 14, members: 40 */
/* sum members: 837, holes: 4, sum holes: 19 */
/* bit holes: 1, sum bit holes: 29 bits */
/* last cacheline: 24 bytes */

Without patch:
/* size: 864, cachelines: 14, members: 41 */
/* sum members: 841, holes: 5, sum holes: 23 */
/* bit holes: 1, sum bit holes: 29 bits */
/* last cacheline: 32 bytes */

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
Reviewed-by: David Sterba <dsterba@suse.com>
---

Changes since v2:
 * Add SOB and Reviewed-by tags

Changes since v1: 
 * Missed the usage in btrfs-tests.c

 fs/btrfs/ctree.h             | 1 -
 fs/btrfs/extent-tree.c       | 1 -
 fs/btrfs/tests/btrfs-tests.c | 1 -
 3 files changed, 3 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index a75a23f9d68e..cdd3775e930b 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -559,7 +559,6 @@ struct btrfs_block_group_cache {
 	u64 bytes_super;
 	u64 flags;
 	u64 cache_generation;
-	u32 sectorsize;
 
 	/*
 	 * If the free space extent count exceeds this number, convert the block
diff --git a/fs/btrfs/extent-tree.c b/fs/btrfs/extent-tree.c
index a08a743a8e09..2a0d300c7d1a 100644
--- a/fs/btrfs/extent-tree.c
+++ b/fs/btrfs/extent-tree.c
@@ -9904,7 +9904,6 @@ btrfs_create_block_group_cache(struct btrfs_fs_info *fs_info,
 	cache->key.offset = size;
 	cache->key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
 
-	cache->sectorsize = fs_info->sectorsize;
 	cache->fs_info = fs_info;
 	cache->full_stripe_len = btrfs_full_stripe_len(fs_info,
 						       &fs_info->mapping_tree,
diff --git a/fs/btrfs/tests/btrfs-tests.c b/fs/btrfs/tests/btrfs-tests.c
index b18ab8f327a5..d3f25376a0f8 100644
--- a/fs/btrfs/tests/btrfs-tests.c
+++ b/fs/btrfs/tests/btrfs-tests.c
@@ -211,7 +211,6 @@ btrfs_alloc_dummy_block_group(struct btrfs_fs_info *fs_info,
 	cache->key.objectid = 0;
 	cache->key.offset = length;
 	cache->key.type = BTRFS_BLOCK_GROUP_ITEM_KEY;
-	cache->sectorsize = fs_info->sectorsize;
 	cache->full_stripe_len = fs_info->sectorsize;
 	cache->fs_info = fs_info;
 
-- 
2.7.4

Re: [PATCH v3] btrfs: remove unused sectorsize member

[David Sterba]

On Wed, Jun 28, 2017 at 11:05:22AM +0300, Nikolay Borisov wrote:
> The sectorsize member of btrfs_block_group_cache is unused. So remove it, this
> reduces the number of holes in the struct.
> 
> With patch:
> /* size: 856, cachelines: 14, members: 40 */
> /* sum members: 837, holes: 4, sum holes: 19 */
> /* bit holes: 1, sum bit holes: 29 bits */
> /* last cacheline: 24 bytes */
> 
> Without patch:
> /* size: 864, cachelines: 14, members: 41 */
> /* sum members: 841, holes: 5, sum holes: 23 */
> /* bit holes: 1, sum bit holes: 29 bits */
> /* last cacheline: 32 bytes */
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> Reviewed-by: David Sterba <dsterba@suse.com>

Strange that the error appeared just now, the code hasn't been modified
recently.

  CC [M]  fs/btrfs/tests/free-space-tree-tests.o
fs/btrfs/tests/free-space-tree-tests.c: In function ‘__check_free_space_extents’:
fs/btrfs/tests/free-space-tree-tests.c:84:20: error: ‘struct btrfs_block_group_cache’ has no member named ‘sectorsize’
     offset += cache->sectorsize;
                    ^~
make[1]: *** [scripts/Makefile.build:302: fs/btrfs/tests/free-space-tree-tests.o] Error 1
make: *** [Makefile:1684: fs/btrfs/] Error 2

I'll fix it.