[PATCH 0/3] Cleanup short int types in block group reserve

[PATCH 0/3] Cleanup short int types in block group reserve

[David Sterba]

Using the short type in btrfs_block_rsv is not needed for bool
indicators and we can make the structure smaller.

David Sterba (3):
  btrfs: switch btrfs_block_rsv::full to bool
  btrfs: switch btrfs_block_rsv::failfast to bool
  btrfs: use u8 type for btrfs_block_rsv::type

 fs/btrfs/block-rsv.c   | 22 +++++++++-------------
 fs/btrfs/block-rsv.h   | 14 +++++++-------
 fs/btrfs/delayed-ref.c |  4 ++--
 fs/btrfs/file.c        |  2 +-
 fs/btrfs/inode.c       |  4 ++--
 5 files changed, 21 insertions(+), 25 deletions(-)

-- 
2.36.1

[PATCH 1/3] btrfs: switch btrfs_block_rsv::full to bool

[David Sterba]

Use simple bool type for the block reserve full status, there's short to
save space as there used to be int but there's no reason for that.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/block-rsv.c   | 15 ++++++---------
 fs/btrfs/block-rsv.h   |  2 +-
 fs/btrfs/delayed-ref.c |  4 ++--
 3 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
index b3ee49b0b1e8..26c43a6ef5d2 100644
--- a/fs/btrfs/block-rsv.c
+++ b/fs/btrfs/block-rsv.c
@@ -118,7 +118,7 @@ static u64 block_rsv_release_bytes(struct btrfs_fs_info *fs_info,
 	if (block_rsv->reserved >= block_rsv->size) {
 		num_bytes = block_rsv->reserved - block_rsv->size;
 		block_rsv->reserved = block_rsv->size;
-		block_rsv->full = 1;
+		block_rsv->full = true;
 	} else {
 		num_bytes = 0;
 	}
@@ -142,7 +142,7 @@ static u64 block_rsv_release_bytes(struct btrfs_fs_info *fs_info,
 				bytes_to_add = min(num_bytes, bytes_to_add);
 				dest->reserved += bytes_to_add;
 				if (dest->reserved >= dest->size)
-					dest->full = 1;
+					dest->full = true;
 				num_bytes -= bytes_to_add;
 			}
 			spin_unlock(&dest->lock);
@@ -304,7 +304,7 @@ int btrfs_block_rsv_use_bytes(struct btrfs_block_rsv *block_rsv, u64 num_bytes)
 	if (block_rsv->reserved >= num_bytes) {
 		block_rsv->reserved -= num_bytes;
 		if (block_rsv->reserved < block_rsv->size)
-			block_rsv->full = 0;
+			block_rsv->full = false;
 		ret = 0;
 	}
 	spin_unlock(&block_rsv->lock);
@@ -319,7 +319,7 @@ void btrfs_block_rsv_add_bytes(struct btrfs_block_rsv *block_rsv,
 	if (update_size)
 		block_rsv->size += num_bytes;
 	else if (block_rsv->reserved >= block_rsv->size)
-		block_rsv->full = 1;
+		block_rsv->full = true;
 	spin_unlock(&block_rsv->lock);
 }
 
@@ -341,7 +341,7 @@ int btrfs_cond_migrate_bytes(struct btrfs_fs_info *fs_info,
 	}
 	global_rsv->reserved -= num_bytes;
 	if (global_rsv->reserved < global_rsv->size)
-		global_rsv->full = 0;
+		global_rsv->full = false;
 	spin_unlock(&global_rsv->lock);
 
 	btrfs_block_rsv_add_bytes(dest, num_bytes, true);
@@ -408,10 +408,7 @@ void btrfs_update_global_block_rsv(struct btrfs_fs_info *fs_info)
 		btrfs_try_granting_tickets(fs_info, sinfo);
 	}
 
-	if (block_rsv->reserved == block_rsv->size)
-		block_rsv->full = 1;
-	else
-		block_rsv->full = 0;
+	block_rsv->full = (block_rsv->reserved == block_rsv->size);
 
 	if (block_rsv->size >= sinfo->total_bytes)
 		sinfo->force_alloc = CHUNK_ALLOC_FORCE;
diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
index 3b67ff08d434..99c491ef128e 100644
--- a/fs/btrfs/block-rsv.h
+++ b/fs/btrfs/block-rsv.h
@@ -25,7 +25,7 @@ struct btrfs_block_rsv {
 	u64 reserved;
 	struct btrfs_space_info *space_info;
 	spinlock_t lock;
-	unsigned short full;
+	bool full;
 	unsigned short type;
 	unsigned short failfast;
 
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 99f37fca2e96..7c455d2d81c0 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -132,7 +132,7 @@ void btrfs_update_delayed_refs_rsv(struct btrfs_trans_handle *trans)
 
 	spin_lock(&delayed_rsv->lock);
 	delayed_rsv->size += num_bytes;
-	delayed_rsv->full = 0;
+	delayed_rsv->full = false;
 	spin_unlock(&delayed_rsv->lock);
 	trans->delayed_ref_updates = 0;
 }
@@ -175,7 +175,7 @@ void btrfs_migrate_to_delayed_refs_rsv(struct btrfs_fs_info *fs_info,
 	if (num_bytes)
 		delayed_refs_rsv->reserved += num_bytes;
 	if (delayed_refs_rsv->reserved >= delayed_refs_rsv->size)
-		delayed_refs_rsv->full = 1;
+		delayed_refs_rsv->full = false;
 	spin_unlock(&delayed_refs_rsv->lock);
 
 	if (num_bytes)
-- 
2.36.1

Re: [PATCH 1/3] btrfs: switch btrfs_block_rsv::full to bool

[Antonio Pérez]

Hi!

El 24/6/22 a las 16:01, David Sterba escribió:

> @@ -175,7 +175,7 @@ void btrfs_migrate_to_delayed_refs_rsv(struct btrfs_fs_info *fs_info,
>   	if (num_bytes)
>   		delayed_refs_rsv->reserved += num_bytes;
>   	if (delayed_refs_rsv->reserved >= delayed_refs_rsv->size)
> -		delayed_refs_rsv->full = 1;
> +		delayed_refs_rsv->full = false;

Should it be 'true'?

Thanks,
Antonio

Re: [PATCH 1/3] btrfs: switch btrfs_block_rsv::full to bool

[David Sterba]

On Fri, Jun 24, 2022 at 07:12:09PM +0200, Antonio Pérez wrote:
> Hi!
> 
> El 24/6/22 a las 16:01, David Sterba escribió:
> 
> > @@ -175,7 +175,7 @@ void btrfs_migrate_to_delayed_refs_rsv(struct btrfs_fs_info *fs_info,
> >   	if (num_bytes)
> >   		delayed_refs_rsv->reserved += num_bytes;
> >   	if (delayed_refs_rsv->reserved >= delayed_refs_rsv->size)
> > -		delayed_refs_rsv->full = 1;
> > +		delayed_refs_rsv->full = false;
> 
> Should it be 'true'?

Yes of course, thanks

[PATCH 2/3] btrfs: switch btrfs_block_rsv::failfast to bool

[David Sterba]

Use simple bool type for the block reserve failfast status, there's
short to save space as there used to be int but there's no reason for
that.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/block-rsv.h | 2 +-
 fs/btrfs/file.c      | 2 +-
 fs/btrfs/inode.c     | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
index 99c491ef128e..0702d4087ff6 100644
--- a/fs/btrfs/block-rsv.h
+++ b/fs/btrfs/block-rsv.h
@@ -26,8 +26,8 @@ struct btrfs_block_rsv {
 	struct btrfs_space_info *space_info;
 	spinlock_t lock;
 	bool full;
+	bool failfast;
 	unsigned short type;
-	unsigned short failfast;
 
 	/*
 	 * Qgroup equivalent for @size @reserved
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index 89c6d7ff1987..3e7f89d50db9 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2734,7 +2734,7 @@ int btrfs_replace_file_extents(struct btrfs_inode *inode,
 		goto out;
 	}
 	rsv->size = btrfs_calc_insert_metadata_size(fs_info, 1);
-	rsv->failfast = 1;
+	rsv->failfast = true;
 
 	/*
 	 * 1 - update the inode
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index 07b3fb90b621..1b2df600681b 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -5432,7 +5432,7 @@ void btrfs_evict_inode(struct inode *inode)
 	if (!rsv)
 		goto no_delete;
 	rsv->size = btrfs_calc_metadata_size(fs_info, 1);
-	rsv->failfast = 1;
+	rsv->failfast = true;
 
 	btrfs_i_size_write(BTRFS_I(inode), 0);
 
@@ -8676,7 +8676,7 @@ static int btrfs_truncate(struct inode *inode, bool skip_writeback)
 	if (!rsv)
 		return -ENOMEM;
 	rsv->size = min_size;
-	rsv->failfast = 1;
+	rsv->failfast = true;
 
 	/*
 	 * 1 for the truncate slack space
-- 
2.36.1

Re: [PATCH 2/3] btrfs: switch btrfs_block_rsv::failfast to bool

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

[PATCH 3/3] btrfs: use u8 type for btrfs_block_rsv::type

[David Sterba]

The number of block group reserve types BTRFS_BLOCK_RSV_* is small and
fits to u8 and there's enough left in case we want to add more.

The structure size is now 48 on release build, making a slight
improvement in structures where it's embedded, like btrfs_fs_info or
btrfs_inode.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/block-rsv.c |  7 +++----
 fs/btrfs/block-rsv.h | 10 +++++-----
 2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/block-rsv.c b/fs/btrfs/block-rsv.c
index 26c43a6ef5d2..5384c6ae8fe8 100644
--- a/fs/btrfs/block-rsv.c
+++ b/fs/btrfs/block-rsv.c
@@ -171,7 +171,7 @@ int btrfs_block_rsv_migrate(struct btrfs_block_rsv *src,
 	return 0;
 }
 
-void btrfs_init_block_rsv(struct btrfs_block_rsv *rsv, unsigned short type)
+void btrfs_init_block_rsv(struct btrfs_block_rsv *rsv, u8 type)
 {
 	memset(rsv, 0, sizeof(*rsv));
 	spin_lock_init(&rsv->lock);
@@ -179,8 +179,7 @@ void btrfs_init_block_rsv(struct btrfs_block_rsv *rsv, unsigned short type)
 }
 
 void btrfs_init_metadata_block_rsv(struct btrfs_fs_info *fs_info,
-				   struct btrfs_block_rsv *rsv,
-				   unsigned short type)
+				   struct btrfs_block_rsv *rsv, u8 type)
 {
 	btrfs_init_block_rsv(rsv, type);
 	rsv->space_info = btrfs_find_space_info(fs_info,
@@ -188,7 +187,7 @@ void btrfs_init_metadata_block_rsv(struct btrfs_fs_info *fs_info,
 }
 
 struct btrfs_block_rsv *btrfs_alloc_block_rsv(struct btrfs_fs_info *fs_info,
-					      unsigned short type)
+					      u8 type)
 {
 	struct btrfs_block_rsv *block_rsv;
 
diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
index 0702d4087ff6..bb449c75ee4c 100644
--- a/fs/btrfs/block-rsv.h
+++ b/fs/btrfs/block-rsv.h
@@ -27,7 +27,8 @@ struct btrfs_block_rsv {
 	spinlock_t lock;
 	bool full;
 	bool failfast;
-	unsigned short type;
+	/* Block reserve type, one of BTRFS_BLOCK_RSV_* */
+	u8 type;
 
 	/*
 	 * Qgroup equivalent for @size @reserved
@@ -49,13 +50,12 @@ struct btrfs_block_rsv {
 	u64 qgroup_rsv_reserved;
 };
 
-void btrfs_init_block_rsv(struct btrfs_block_rsv *rsv, unsigned short type);
+void btrfs_init_block_rsv(struct btrfs_block_rsv *rsv, u8 type);
 void btrfs_init_root_block_rsv(struct btrfs_root *root);
 struct btrfs_block_rsv *btrfs_alloc_block_rsv(struct btrfs_fs_info *fs_info,
-					      unsigned short type);
+					      u8 type);
 void btrfs_init_metadata_block_rsv(struct btrfs_fs_info *fs_info,
-				   struct btrfs_block_rsv *rsv,
-				   unsigned short type);
+				   struct btrfs_block_rsv *rsv, u8 type);
 void btrfs_free_block_rsv(struct btrfs_fs_info *fs_info,
 			  struct btrfs_block_rsv *rsv);
 int btrfs_block_rsv_add(struct btrfs_fs_info *fs_info,
-- 
2.36.1

Re: [PATCH 3/3] btrfs: use u8 type for btrfs_block_rsv::type

[Johannes Thumshirn]

On 24.06.22 16:15, David Sterba wrote:
> diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
> index 0702d4087ff6..bb449c75ee4c 100644
> --- a/fs/btrfs/block-rsv.h
> +++ b/fs/btrfs/block-rsv.h
> @@ -27,7 +27,8 @@ struct btrfs_block_rsv {
>  	spinlock_t lock;
>  	bool full;
>  	bool failfast;
> -	unsigned short type;
> +	/* Block reserve type, one of BTRFS_BLOCK_RSV_* */
> +	u8 type;
>  

Is there any reason to not use the enum?

Re: [PATCH 3/3] btrfs: use u8 type for btrfs_block_rsv::type

[David Sterba]

On Mon, Jun 27, 2022 at 06:51:30AM +0000, Johannes Thumshirn wrote:
> On 24.06.22 16:15, David Sterba wrote:
> > diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
> > index 0702d4087ff6..bb449c75ee4c 100644
> > --- a/fs/btrfs/block-rsv.h
> > +++ b/fs/btrfs/block-rsv.h
> > @@ -27,7 +27,8 @@ struct btrfs_block_rsv {
> >  	spinlock_t lock;
> >  	bool full;
> >  	bool failfast;
> > -	unsigned short type;
> > +	/* Block reserve type, one of BTRFS_BLOCK_RSV_* */
> > +	u8 type;
> >  
> 
> Is there any reason to not use the enum?

Enum would be 'int', 4 bytes to the space optimization would be
lost. Enum types can be shortened as

	enum btrfs_reserve type:8

but I'm not sure it's an improvement.

Re: [PATCH 3/3] btrfs: use u8 type for btrfs_block_rsv::type

[Johannes Thumshirn]

On 27.06.22 18:45, David Sterba wrote:
> On Mon, Jun 27, 2022 at 06:51:30AM +0000, Johannes Thumshirn wrote:
>> On 24.06.22 16:15, David Sterba wrote:
>>> diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
>>> index 0702d4087ff6..bb449c75ee4c 100644
>>> --- a/fs/btrfs/block-rsv.h
>>> +++ b/fs/btrfs/block-rsv.h
>>> @@ -27,7 +27,8 @@ struct btrfs_block_rsv {
>>>  	spinlock_t lock;
>>>  	bool full;
>>>  	bool failfast;
>>> -	unsigned short type;
>>> +	/* Block reserve type, one of BTRFS_BLOCK_RSV_* */
>>> +	u8 type;
>>>  
>>
>> Is there any reason to not use the enum?
> 
> Enum would be 'int', 4 bytes to the space optimization would be
> lost. Enum types can be shortened as
> 
> 	enum btrfs_reserve type:8
> 
> but I'm not sure it's an improvement.
> 

Using an enum would give some type safety (I think -Wenum-compare is 
on by default in the kernel). Packing that enum would give us the 1 byte
size you're looking for.

Re: [PATCH 3/3] btrfs: use u8 type for btrfs_block_rsv::type

[David Sterba]

On Tue, Jun 28, 2022 at 07:15:14AM +0000, Johannes Thumshirn wrote:
> On 27.06.22 18:45, David Sterba wrote:
> > On Mon, Jun 27, 2022 at 06:51:30AM +0000, Johannes Thumshirn wrote:
> >> On 24.06.22 16:15, David Sterba wrote:
> >>> diff --git a/fs/btrfs/block-rsv.h b/fs/btrfs/block-rsv.h
> >>> index 0702d4087ff6..bb449c75ee4c 100644
> >>> --- a/fs/btrfs/block-rsv.h
> >>> +++ b/fs/btrfs/block-rsv.h
> >>> @@ -27,7 +27,8 @@ struct btrfs_block_rsv {
> >>>  	spinlock_t lock;
> >>>  	bool full;
> >>>  	bool failfast;
> >>> -	unsigned short type;
> >>> +	/* Block reserve type, one of BTRFS_BLOCK_RSV_* */
> >>> +	u8 type;
> >>>  
> >>
> >> Is there any reason to not use the enum?
> > 
> > Enum would be 'int', 4 bytes to the space optimization would be
> > lost. Enum types can be shortened as
> > 
> > 	enum btrfs_reserve type:8
> > 
> > but I'm not sure it's an improvement.
> > 
> 
> Using an enum would give some type safety (I think -Wenum-compare is 
> on by default in the kernel). Packing that enum would give us the 1 byte
> size you're looking for.

Yeah I'll go with the named enum and :8 in the structure.