2023-06-21 09:26:16

by Li Nan

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

From: Li Nan <[email protected]>

"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 <[email protected]>
---
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) {
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 */
--
2.39.2

2023-06-21 14:11:07

by Ashok Raj

Subject: Re: [PATCH v3 1/4] block/badblocks: change some members of badblocks to bool

On Thu, Jun 22, 2023 at 01:20:49AM +0800, [email protected] wrote:
> From: Li Nan <[email protected]>
>
> "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 <[email protected]>
> ---
> 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
>

2023-06-25 09:55:58

by Li Nan

Subject: Re: [PATCH v3 1/4] block/badblocks: change some members of badblocks to bool

在 2023/6/21 22:02, Ashok Raj 写道:
> On Thu, Jun 22, 2023 at 01:20:49AM +0800, [email protected] wrote:
>> From: Li Nan <[email protected]>
>>
>> "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 <[email protected]>
>> ---
>> 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
>>
>
> .

--
Thanks,
Nan