Message-ID: <CAKUZ0zKp0r0ydTSFrdkUBzG+rWXDvP5uwVYZ+C8Xwy9wvZU-3w@mail.gmail.com>
Date: Tue, 8 Apr 2025 09:27:00 -0400
From: Gabriel Shahrouzi <gshahrouzi@...il.com>
To: Coly Li <colyli@...nel.org>
Cc: Kent Overstreet <kent.overstreet@...ux.dev>, linux-bcache@...r.kernel.org, 
	linux-kernel@...r.kernel.org, skhan@...uxfoundation.org, 
	kernelmentees@...ts.linuxfoundation.org
Subject: Re: [PATCH 2/2] bcache: Fix warnings for incorrect type in assignments

On Tue, Apr 8, 2025 at 6:39 AM Coly Li <colyli@...nel.org> wrote:
>
> 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@...nel.org> 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.
Got it.
>
> > 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.
Ah I should have been more clear about this when explaining. My main
concern was with the endian conversion which is what prompted me to
group them together.
>
> 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.
Ah ok this makes sense when considering the use case of bcache.
>
> 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.
Makes sense.
>
> Thanks.
>
> > > > Signed-off-by: Gabriel Shahrouzi <gshahrouzi@...il.com>
> > > > ---
> > > >  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

Powered by blists - more mailing lists