Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

From: Coly Li
Date: Tue Apr 08 2025 - 06:44:14 EST

On Tue, Apr 08, 2025 at 03:15:00AM +0800, Gabriel Shahrouzi wrote:
> On Tue, Apr 8, 2025 at 12:58 AM Coly Li <colyli@xxxxxxxxxx> wrote:
> >
> > On Mon, Apr 07, 2025 at 11:33:22PM +0800, Gabriel Shahrouzi wrote:
> > > Remove unnecessary cpu_to_le16() and cpu_to_le32() conversions when
> > > assigning values (priorities, timestamps) to native integer type
> > > members. Prevent incorrect byte ordering for big-endian systems.
> > >
> >
> > Hmm, why do you feel the conversions are unncessary? Please explain
> > with details.
> I used Sparse for static analysis on bcache and it gave incorrect type
> in assignment warnings.
>
> For example:
>
> u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
>
> ktime_get_real_seconds() returns back u64 and gets casted down to a
> u32. u is of type struct uuid_entry whose member fields are either u8,
> u32, or u64. A conversion here contradicts the type it should be
> assigned.
>
> From my understanding, this would not produce an unexpected result if
> the value were to be read from or written to some location which seems
> to be the case here. I believe it would only cause issues on
> big-endian systems if the value were to be modified in some way.
>

Yes you are right, and I agree with you.


> Looking at the commit history for when the code for this specific
> example was first introduced (12 years ago), it seems like this was
> the author’s intent. It looks like the intention was to store the
> value as little endian in uint32_t. Doing this, the author saves space
> / time. If the type was le32 instead, the conversion would have to be
> applied each time it’s used. Alternatively, if another member variable
> was defined but for the le32 version, then extra space is used up.
>

This is kind of convention that on-media values are stored in little
endian, for portablity purpose. But bcache is special, current
implementation and usage don't require/support portability on different
byte order machines. So cpu_to/from_le** routines are almostly
unnecessary indeed.

*BUT* the cast (u32) works as expected on big endian machine as well,
same result generated as little indian machine does. The out-of-order
issue on big endian machine for the code you mentioned won't happen.

> In the unlikely event that these specific files change drastically,
> making sure the types are the same serves as a preventative measure
> to make sure it’s not misused. On the other hand, making the change
> most likely goes against the author’s original intent and could cause
> something unintended.
> >
> > I don't mean the modification is correct or incorrect, just want to
> > see detailed analysis and help me understand in correct why as you
> > are.
> >
> > BTW, did you have chance to test your patch on big-endian machine?
> I only analyzed the compilation warnings so far. I’ll look into trying
> to test this on a big-endian machine.
>
>

You may have a try and verify my statement.

And for the change in bch_prio_write(), this is something out of your
orignal scope of this patch. The prio width is 16bits, byte order and
length truncation issue doesn't apply here.

After all, no mather the cpu_to_le*() or le*_to_cpu() routines are used
or not, the code works well. Because bcache cache device dosn't port
between big and little endian machines.

I don't want to unify the code to all use cpu_to_le*() routines or
remove all these routines, both sides make sense and resonable.
IMHO they are just changes for changing. So I intend to keep it as what
Kent orignally wrote it.

Thanks.

> > > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@xxxxxxxxx>
> > > ---
> > > drivers/md/bcache/super.c | 12 ++++++------
> > > 1 file changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/md/bcache/super.c b/drivers/md/bcache/super.c
> > > index e42f1400cea9d..c4c5ca17fb600 100644
> > > --- a/drivers/md/bcache/super.c
> > > +++ b/drivers/md/bcache/super.c
> > > @@ -648,7 +648,7 @@ int bch_prio_write(struct cache *ca, bool wait)
> > > for (b = ca->buckets + i * prios_per_bucket(ca);
> > > b < ca->buckets + ca->sb.nbuckets && d < end;
> > > b++, d++) {
> > > - d->prio = cpu_to_le16(b->prio);
> > > + d->prio = b->prio;
> > > d->gen = b->gen;
> > > }
> > >
> > > @@ -721,7 +721,7 @@ static int prio_read(struct cache *ca, uint64_t bucket)
> > > d = p->data;
> > > }
> > >
> > > - b->prio = le16_to_cpu(d->prio);
> > > + b->prio = d->prio;
> > > b->gen = b->last_gc = d->gen;
> > > }
> > >
> > > @@ -832,7 +832,7 @@ static void bcache_device_detach(struct bcache_device *d)
> > >
> > > SET_UUID_FLASH_ONLY(u, 0);
> > > memcpy(u->uuid, invalid_uuid, 16);
> > > - u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > > + u->invalidated = (u32)ktime_get_real_seconds();
> > > bch_uuid_write(d->c);
> > > }
> > >
> > > @@ -1188,7 +1188,7 @@ void bch_cached_dev_detach(struct cached_dev *dc)
> > > int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> > > uint8_t *set_uuid)
> > > {
> > > - uint32_t rtime = cpu_to_le32((u32)ktime_get_real_seconds());
> > > + uint32_t rtime = (u32)ktime_get_real_seconds();
> > > struct uuid_entry *u;
> > > struct cached_dev *exist_dc, *t;
> > > int ret = 0;
> > > @@ -1230,7 +1230,7 @@ int bch_cached_dev_attach(struct cached_dev *dc, struct cache_set *c,
> > > (BDEV_STATE(&dc->sb) == BDEV_STATE_STALE ||
> > > BDEV_STATE(&dc->sb) == BDEV_STATE_NONE)) {
> > > memcpy(u->uuid, invalid_uuid, 16);
> > > - u->invalidated = cpu_to_le32((u32)ktime_get_real_seconds());
> > > + u->invalidated = (u32)ktime_get_real_seconds();
> > > u = NULL;
> > > }
> > >
> > > @@ -1591,7 +1591,7 @@ int bch_flash_dev_create(struct cache_set *c, uint64_t size)
> > >
> > > get_random_bytes(u->uuid, 16);
> > > memset(u->label, 0, 32);
> > > - u->first_reg = u->last_reg = cpu_to_le32((u32)ktime_get_real_seconds());
> > > + u->first_reg = u->last_reg = (u32)ktime_get_real_seconds();
> > >
> > > SET_UUID_FLASH_ONLY(u, 1);
> > > u->sectors = size >> 9;
> > > --
> > > 2.43.0
> > >
> >
> > --
> > Coly Li
>

--
Coly Li