Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210915035227.630204-1-linux@roeck-us.net> <CAHk-=whSkMh9mc7+OSBZZvpoEEJmS6qY7kX3qixEXTLKGc=wgw@mail.gmail.com>
 <CAHk-=wjynK7SSgTOvW7tfpFZZ0pzo67BsOsqtVHYtvju8F_bng@mail.gmail.com> <5497691.DvuYhMxLoT@alarsen.net>
In-Reply-To: <5497691.DvuYhMxLoT@alarsen.net>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 16 Sep 2021 09:25:38 -0700
Message-ID: <CAHk-=wh84ks6FN2fBWrGZNKNhOmTZL-r5xZG7gYZ==jESG2GgA@mail.gmail.com>
Subject: Re: [PATCH v2 0/4] Introduce and use absolute_pointer macro
To:     Anders Larsen <al@alarsen.net>
Cc:     Guenter Roeck <linux@roeck-us.net>,
        Richard Henderson <rth@twiddle.net>,
        Ivan Kokshaysky <ink@jurassic.park.msu.ru>,
        Matt Turner <mattst88@gmail.com>,
        "James E . J . Bottomley" <James.Bottomley@hansenpartnership.com>,
        Helge Deller <deller@gmx.de>,
        "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        alpha <linux-alpha@vger.kernel.org>,
        Geert Uytterhoeven <geert@linux-m68k.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-parisc@vger.kernel.org, Netdev <netdev@vger.kernel.org>,
        Sparse Mailing-list <linux-sparse@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Sep 16, 2021 at 12:02 AM Anders Larsen <al@alarsen.net> wrote:
>
> On Wednesday, 2021-09-15 23:19 Linus Torvalds wrote:
> >
> > But hey, maybe it just works so well for the very specialized user base ...
>
> it's actually the latter (although I guess the user base is shrinking)

Hey, so if it's actively used, maybe you can answer a question or two
that I have just because I looked at the code..

In particular, the inode number calculation is odd. Is there a reason
for the "-1"? Because iboth the link case and the direct inode case
have it, but t's a _different_ "-1":

For the "inode_entry", it does

                ino = blknum * QNX4_INODES_PER_BLOCK + ix - 1;

but it's worth noting that "ix" is zero-based (index within the
block), so this kind of oddly removes one from a zero-based thing, and
the 'ino' for the very first entry ends up being -1.

Of course, it's possible that the first entry is always empty, but it
does seem a bit odd.

For the "link_info" case, it does

            ino = ( le32_to_cpu(de->link.dl_inode_blk) - 1 ) *
                    QNX4_INODES_PER_BLOCK +
                    de->link.dl_inode_ndx;

so now it takes the _block_ index, and does that "-1" on it, and then
multiplies it by the "entries per block" number, and adds the index.

So now if both are zero, the inode number is -8, not -1.

But all of this matches what the *lookup* code does. It's very odd, though.

But to make it stranger, then in "qnx4_iget()", the calculations all
makes sense. There it just does "take the inode number, and look up
block and index into the block using it".

Very strange and confusing. Because it means that iget() seems to look
up a *different* inode entry than "lookup" and "readdir" actually look
at.

I must be missing something. I obviously didn't touch any of this
logic, I was just doing the "make the type system clearer for the
compiler".

Also, I have to say, since I was looking at compiler output, the
calculations in readdir() are made much worse by the fact that the
dir->pos is a "loff_t". That's signed. And then you use "%" to get the
index within a block. Using '%' instead of bitops is fairly
equivalent, but only for

 (a) unsigned types

 (b) when the divisor is a compile-time power-of-2

In the qnx4 case, (b) is true, but (a) is not.

Not a big deal. But usually, I tell people to avoid '% ENTRIES',
because it really has very different behavior from '& MASK' for signed
numbers.

                  Linus