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

[Li Nan]

在 2023/6/21 22:02, Ashok Raj 写道:
> On Thu, Jun 22, 2023 at 01:20:49AM +0800, linan666@xxxxxxxxxxxxxxx wrote:
> > From: Li Nan <linan122@xxxxxxxxxx>
> >
> > "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@xxxxxxxxxx>
> > ---
> >   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