diff mbox series

[v3,1/4] block/badblocks: change some members of badblocks to bool

Message ID 20230621172052.1499919-2-linan666@huaweicloud.com
State New
Headers
Series block/badblocks: fix badblocks setting error |

Commit Message

Li Nan June 21, 2023, 5:20 p.m. UTC 
  From: Li Nan <linan122@huawei.com>

"changed" and "unacked_exist" are used as boolean type. Change the type
of them to bool. And reorder fields to reduce memory hole.

No functional changed intended.

Signed-off-by: Li Nan <linan122@huawei.com>
---
 block/badblocks.c         | 14 +++++++-------
 include/linux/badblocks.h | 10 +++++-----
 2 files changed, 12 insertions(+), 12 deletions(-)

Comments

Ashok Raj June 21, 2023, 2:02 p.m. UTC | #1 
On Thu, Jun 22, 2023 at 01:20:49AM +0800, linan666@huaweicloud.com wrote:
> From: Li Nan <linan122@huawei.com>
> 
> "changed" and "unacked_exist" are used as boolean type. Change the type
> of them to bool. And reorder fields to reduce memory hole.

minor nit: If you use a .gitorderfile to list .h before .c it will help review them in
order.

I don't know if its even worth doing this manual compaction unless you are
storing the entire struct in some flash or its in a sensitive cache
thrashing structure.

bool is useful that it makes the code easier to read and can eliminate some
class of bugs that you would otherwise use !! operator.

> 
> No functional changed intended.
> 
> Signed-off-by: Li Nan <linan122@huawei.com>
> ---
>  block/badblocks.c         | 14 +++++++-------
>  include/linux/badblocks.h | 10 +++++-----
>  2 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/block/badblocks.c b/block/badblocks.c
> index 3afb550c0f7b..1b4caa42c5f1 100644
> --- a/block/badblocks.c
> +++ b/block/badblocks.c
> @@ -141,7 +141,7 @@ static void badblocks_update_acked(struct badblocks *bb)
>  	}
>  
>  	if (!unacked)
> -		bb->unacked_exist = 0;
> +		bb->unacked_exist = false;
>  }
>  
>  /**
> @@ -302,9 +302,9 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>  		}
>  	}
>  
> -	bb->changed = 1;
> +	bb->changed = true;
>  	if (!acknowledged)
> -		bb->unacked_exist = 1;
> +		bb->unacked_exist = true;
>  	else
>  		badblocks_update_acked(bb);
>  	write_sequnlock_irqrestore(&bb->lock, flags);
> @@ -414,7 +414,7 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>  	}
>  
>  	badblocks_update_acked(bb);
> -	bb->changed = 1;
> +	bb->changed = true;
>  out:
>  	write_sequnlock_irq(&bb->lock);
>  	return rv;
> @@ -435,7 +435,7 @@ void ack_all_badblocks(struct badblocks *bb)
>  		return;
>  	write_seqlock_irq(&bb->lock);
>  
> -	if (bb->changed == 0 && bb->unacked_exist) {
> +	if (bb->changed == false && bb->unacked_exist) {

	if (!bb->changed && bb->unacked_exist)


>  		u64 *p = bb->page;
>  		int i;
>  
> @@ -447,7 +447,7 @@ void ack_all_badblocks(struct badblocks *bb)
>  				p[i] = BB_MAKE(start, len, 1);
>  			}
>  		}
> -		bb->unacked_exist = 0;
> +		bb->unacked_exist = false;
>  	}
>  	write_sequnlock_irq(&bb->lock);
>  }
> @@ -493,7 +493,7 @@ ssize_t badblocks_show(struct badblocks *bb, char *page, int unack)
>  				length << bb->shift);
>  	}
>  	if (unack && len == 0)
> -		bb->unacked_exist = 0;
> +		bb->unacked_exist = false;
>  
>  	if (read_seqretry(&bb->lock, seq))
>  		goto retry;
> diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
> index 2426276b9bd3..c2723f97d22d 100644
> --- a/include/linux/badblocks.h
> +++ b/include/linux/badblocks.h
> @@ -27,15 +27,15 @@
>  struct badblocks {
>  	struct device *dev;	/* set by devm_init_badblocks */
>  	int count;		/* count of bad blocks */
> -	int unacked_exist;	/* there probably are unacknowledged
> -				 * bad blocks.  This is only cleared
> -				 * when a read discovers none
> -				 */
>  	int shift;		/* shift from sectors to block size
>  				 * a -ve shift means badblocks are
>  				 * disabled.*/
> +	bool unacked_exist;	/* there probably are unacknowledged
> +				 * bad blocks.  This is only cleared
> +				 * when a read discovers none

read of what?

> +				 */
> +	bool changed;
>  	u64 *page;		/* badblock list */
> -	int changed;
>  	seqlock_t lock;
>  	sector_t sector;
>  	sector_t size;		/* in sectors */
> -- 
> 2.39.2
>
Li Nan June 25, 2023, 9:11 a.m. UTC | #2 
在 2023/6/21 22:02, Ashok Raj 写道:
> On Thu, Jun 22, 2023 at 01:20:49AM +0800, linan666@huaweicloud.com wrote:
>> From: Li Nan <linan122@huawei.com>
>>
>> "changed" and "unacked_exist" are used as boolean type. Change the type
>> of them to bool. And reorder fields to reduce memory hole.
> 
> minor nit: If you use a .gitorderfile to list .h before .c it will help review them in
> order.
> 

I will config my git.

> I don't know if its even worth doing this manual compaction unless you are
> storing the entire struct in some flash or its in a sensitive cache
> thrashing structure.
> 

Yeah, it is worthless to manual compaction.

> bool is useful that it makes the code easier to read and can eliminate some
> class of bugs that you would otherwise use !! operator.
> 
>>
>> No functional changed intended.
>>
>> Signed-off-by: Li Nan <linan122@huawei.com>
>> ---
>>   block/badblocks.c         | 14 +++++++-------
>>   include/linux/badblocks.h | 10 +++++-----
>>   2 files changed, 12 insertions(+), 12 deletions(-)
>>
>> diff --git a/block/badblocks.c b/block/badblocks.c
>> index 3afb550c0f7b..1b4caa42c5f1 100644
>> --- a/block/badblocks.c
>> +++ b/block/badblocks.c
>> @@ -141,7 +141,7 @@ static void badblocks_update_acked(struct badblocks *bb)
>>   	}
>>   
>>   	if (!unacked)
>> -		bb->unacked_exist = 0;
>> +		bb->unacked_exist = false;
>>   }
>>   
>>   /**
>> @@ -302,9 +302,9 @@ int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
>>   		}
>>   	}
>>   
>> -	bb->changed = 1;
>> +	bb->changed = true;
>>   	if (!acknowledged)
>> -		bb->unacked_exist = 1;
>> +		bb->unacked_exist = true;
>>   	else
>>   		badblocks_update_acked(bb);
>>   	write_sequnlock_irqrestore(&bb->lock, flags);
>> @@ -414,7 +414,7 @@ int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
>>   	}
>>   
>>   	badblocks_update_acked(bb);
>> -	bb->changed = 1;
>> +	bb->changed = true;
>>   out:
>>   	write_sequnlock_irq(&bb->lock);
>>   	return rv;
>> @@ -435,7 +435,7 @@ void ack_all_badblocks(struct badblocks *bb)
>>   		return;
>>   	write_seqlock_irq(&bb->lock);
>>   
>> -	if (bb->changed == 0 && bb->unacked_exist) {
>> +	if (bb->changed == false && bb->unacked_exist) {
> 
> 	if (!bb->changed && bb->unacked_exist)

I will change it in next version.

> 
> 
>>   		u64 *p = bb->page;
>>   		int i;
>>   
>> @@ -447,7 +447,7 @@ void ack_all_badblocks(struct badblocks *bb)
>>   				p[i] = BB_MAKE(start, len, 1);
>>   			}
>>   		}
>> -		bb->unacked_exist = 0;
>> +		bb->unacked_exist = false;
>>   	}
>>   	write_sequnlock_irq(&bb->lock);
>>   }
>> @@ -493,7 +493,7 @@ ssize_t badblocks_show(struct badblocks *bb, char *page, int unack)
>>   				length << bb->shift);
>>   	}
>>   	if (unack && len == 0)
>> -		bb->unacked_exist = 0;
>> +		bb->unacked_exist = false;
>>   
>>   	if (read_seqretry(&bb->lock, seq))
>>   		goto retry;
>> diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
>> index 2426276b9bd3..c2723f97d22d 100644
>> --- a/include/linux/badblocks.h
>> +++ b/include/linux/badblocks.h
>> @@ -27,15 +27,15 @@
>>   struct badblocks {
>>   	struct device *dev;	/* set by devm_init_badblocks */
>>   	int count;		/* count of bad blocks */
>> -	int unacked_exist;	/* there probably are unacknowledged
>> -				 * bad blocks.  This is only cleared
>> -				 * when a read discovers none
>> -				 */
>>   	int shift;		/* shift from sectors to block size
>>   				 * a -ve shift means badblocks are
>>   				 * disabled.*/
>> +	bool unacked_exist;	/* there probably are unacknowledged
>> +				 * bad blocks.  This is only cleared
>> +				 * when a read discovers none
> 
> read of what?

"... when a read of unacknowledged bad blocks discovers none"

Would this be better？

Thank for your suggestion.

> 
>> +				 */
>> +	bool changed;
>>   	u64 *page;		/* badblock list */
>> -	int changed;
>>   	seqlock_t lock;
>>   	sector_t sector;
>>   	sector_t size;		/* in sectors */
>> -- 
>> 2.39.2
>>
> 
> .
diff mbox series

Patch

diff --git a/block/badblocks.c b/block/badblocks.c
index 3afb550c0f7b..1b4caa42c5f1 100644
--- a/block/badblocks.c
+++ b/block/badblocks.c
@@ -141,7 +141,7 @@  static void badblocks_update_acked(struct badblocks *bb)
 	}
 
 	if (!unacked)
-		bb->unacked_exist = 0;
+		bb->unacked_exist = false;
 }
 
 /**
@@ -302,9 +302,9 @@  int badblocks_set(struct badblocks *bb, sector_t s, int sectors,
 		}
 	}
 
-	bb->changed = 1;
+	bb->changed = true;
 	if (!acknowledged)
-		bb->unacked_exist = 1;
+		bb->unacked_exist = true;
 	else
 		badblocks_update_acked(bb);
 	write_sequnlock_irqrestore(&bb->lock, flags);
@@ -414,7 +414,7 @@  int badblocks_clear(struct badblocks *bb, sector_t s, int sectors)
 	}
 
 	badblocks_update_acked(bb);
-	bb->changed = 1;
+	bb->changed = true;
 out:
 	write_sequnlock_irq(&bb->lock);
 	return rv;
@@ -435,7 +435,7 @@  void ack_all_badblocks(struct badblocks *bb)
 		return;
 	write_seqlock_irq(&bb->lock);
 
-	if (bb->changed == 0 && bb->unacked_exist) {
+	if (bb->changed == false && bb->unacked_exist) {
 		u64 *p = bb->page;
 		int i;
 
@@ -447,7 +447,7 @@  void ack_all_badblocks(struct badblocks *bb)
 				p[i] = BB_MAKE(start, len, 1);
 			}
 		}
-		bb->unacked_exist = 0;
+		bb->unacked_exist = false;
 	}
 	write_sequnlock_irq(&bb->lock);
 }
@@ -493,7 +493,7 @@  ssize_t badblocks_show(struct badblocks *bb, char *page, int unack)
 				length << bb->shift);
 	}
 	if (unack && len == 0)
-		bb->unacked_exist = 0;
+		bb->unacked_exist = false;
 
 	if (read_seqretry(&bb->lock, seq))
 		goto retry;
diff --git a/include/linux/badblocks.h b/include/linux/badblocks.h
index 2426276b9bd3..c2723f97d22d 100644
--- a/include/linux/badblocks.h
+++ b/include/linux/badblocks.h
@@ -27,15 +27,15 @@ 
 struct badblocks {
 	struct device *dev;	/* set by devm_init_badblocks */
 	int count;		/* count of bad blocks */
-	int unacked_exist;	/* there probably are unacknowledged
-				 * bad blocks.  This is only cleared
-				 * when a read discovers none
-				 */
 	int shift;		/* shift from sectors to block size
 				 * a -ve shift means badblocks are
 				 * disabled.*/
+	bool unacked_exist;	/* there probably are unacknowledged
+				 * bad blocks.  This is only cleared
+				 * when a read discovers none
+				 */
+	bool changed;
 	u64 *page;		/* badblock list */
-	int changed;
 	seqlock_t lock;
 	sector_t sector;
 	sector_t size;		/* in sectors */