Re: [PATCH 7/7] lib: remove random.c

["Darrick J. Wong" <djwong@kernel.org>]

On Sun, Mar 16, 2025 at 10:48:50AM -0500, Eric Sandeen wrote:
> On 3/16/25 9:54 AM, Zorro Lang wrote:
> > On Mon, Mar 10, 2025 at 01:29:09PM -0500, Eric Sandeen wrote:
> >> sparse points out that lots of things in random.c could be static,
> >> and upon doing so we realize that nothing in this file is used.
> >> Which is unsurprising since these are all part of the standard
> >> C library ... so just remove the file.
> >>
> >> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> >> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> >> Reviewed-by: Christoph Hellwig <hch@lst.de>
> >> ---
> > 
> > Hi Eric,
> > 
> > When I did the fstests regression test this weekend, I found a regression
> > failure on generic/007 (diff output):
> > 
> >   --- /dev/fd/63	2025-03-15 13:31:35.044534292 -0400
> >   +++ generic/007.out.bad	2025-03-15 13:31:35.002455111 -0400
> >   @@ -14,9 +14,9 @@
> >    .........................................................................
> >    .........................................................................
> >    ....................................................
> >   -creates:  18736 OK,  18802 EEXIST  ( 37538 total, 50% EEXIST)
> >   -removes:  18675 OK,  19927 ENOENT  ( 38602 total, 51% ENOENT)
> >   -lookups:  12000 OK,  11860 ENOENT  ( 23860 total, 49% ENOENT)
> >   -total  :  49411 OK,  50589 w/error (100000 total, 50% w/error)
> >   +creates:  18839 OK,  18890 EEXIST  ( 37729 total, 50% EEXIST)
> >   +removes:  18783 OK,  19951 ENOENT  ( 38734 total, 51% ENOENT)
> >   +lookups:  11858 OK,  11679 ENOENT  ( 23537 total, 49% ENOENT)
> >   +total  :  49480 OK,  50520 w/error (100000 total, 50% w/error)
> >  
> >   -cleanup:     61 removes
> >   +cleanup:     56 removes
> > 
> > By bisecting, the first failed commit is this patch. After removing
> > the fstests internal lib/random.c, the output of src/nametest.c is
> > changed too, that breaks the g/007 (xfs/188 maybe too) test.
> > 
> > It fails on all filesystems (e.g. xfs, ext2/3/4, btrfs, tmpfs, nfs,
> > cifs etc). I'll defer the release of this week (03.16), hope we can
> > fix this regression next week :)
> 
> Oh no, I'm sorry. I thought that if this stuff was never used it'd
> be safe to just yank, but I clearly must have missed something.

Ahahaha, it's linked into libtest.a, which is then linked into every
fstests binary.  Therefore, any binary calling srandom and random get
this bespoke version instead of the one in the C library.  From the
looks of it, the RNG code itself runs the same operations in the same
order every time, which is why you can pass a seed to fsx/fsstress to
get the exact same sequence of operations.

Maybe lib/random.c should have its comment updated?  Clearly two
reviewers, myself, and the patch author missed this point:

/*
 * modified by dxm@sgi.com so that this file acts as a drop in replacement
 * for srandom and random.
 */

Because that doesn't really explain /why/ the code is needed as a drop
in replacement.  How about:

/*
 * modified by dxm@sgi.com so that this file acts as a drop in replacement
 * for srandom and random.  fstests programs rely on the exact sequence
 * of integers generated by these functions for reproducibility and the
 * golden output, which is why we override the C library.
 */

--D

> It's probably best to just revert/remove this patch for now, so it
> doesn't delay any release.
> 
> -Eric
>