On Tue, Feb 13, 2024 at 07:29:32PM -0500, Kent Overstreet wrote:
> On Tue, Feb 13, 2024 at 04:16:34PM -0800, Kees Cook wrote:
> > On Mon, Feb 12, 2024 at 10:54:56AM +1100, Stephen Rothwell wrote:
> > > Hi all,
> > >
> > > After merging the bcachefs tree, today's linux-next build (x86_64
> > > allmodconfig) failed like this:
> > >
> > > ERROR: modpost: missing MODULE_LICENSE() in lib/thread_with_file.o
> > > ERROR: modpost: "stdio_redirect_vprintf" [fs/bcachefs/bcachefs.ko] undefined!
> > > ERROR: modpost: "thread_with_file_exit" [fs/bcachefs/bcachefs.ko] undefined!
> > > ERROR: modpost: "run_thread_with_stdio" [fs/bcachefs/bcachefs.ko] undefined!
> > > ERROR: modpost: "__darray_resize_slowpath" [fs/bcachefs/bcachefs.ko] undefined!
> > > ERROR: modpost: "stdio_redirect_readline" [fs/bcachefs/bcachefs.ko] undefined!
> > > ERROR: modpost: "run_thread_with_file" [fs/bcachefs/bcachefs.ko] undefined!
> > > ERROR: modpost: "__darray_resize_slowpath" [lib/thread_with_file.ko] undefined!
> > >
> > > Caused by commit
> > >
> > > f894f9e5f0ad ("thread_with_file: Lift from bcachefs")
> > >
> > > I have used the version of bcachefs from next-20240206 again.
> >
> > I've mentioned this before, but this patch (and I assume others) was not
> > posted to any mailing list before it appeared in -next. This process
> > failure really needs to be fixed. Please post _everything_ going into
> > your tree to at least linux-bcachefs mailing list, and for things that
> > toss stuff into lib/ it really needs to go to lkml too and CCed to some
> > subset of people who have touched lib/Kconfig, etc last.
>
> thread_wih_file definitely was; the patch moving it to lib/ might not
> have, I'd have to check.
>
> We're having ongoing discussions among us fs developers about how to do
> patch review, and the emerging consensus seems to be that we actually
> don't want to spam the list with every patch (because not every patch is
> interesting!) - we don't want the human-to-human interaction to be
> drowned out on the list.
Then at least CC lkml. Sending to a mailing list isn't optional -- this
is required for Linux development. It's the basis for patch review, how
b4 operates, how tooling finds threads to respond to, etc. It has to go
_somewhere_ before it lands in -next. And using get_maintainers.pl is
the right thing for this -- it'll find the people to CC based on the
MAINTAINERS file, which means more than just file paths, there are "K:"
entries too (e.g. I am CCed on anything using seccomp). This really
needs to happen or you're going to be landing code that didn't get
reviewed fully.
> That doesn't mean we're not doing code review, though! We're
> experimenting with different workflows, there's different thoughts out
> there right now.
If that's true, I would expect to see some "Reviewed-by:" tags, which I
don't see on the patch I mentioned.
The netdev folks have a significantly higher through-put on patches, and
they check all these boxes. Their CI even verifies that emailed patches
are corrected CCed to all expected get_maintainers.pl output, runs
checkpatch, etc. Please please fix this process.
> Regarding CCing people who have touched lib/Kconfig - you sure that's
> the best way to get interested parties who'll do real review? I would
> think review from the people actively working with and using that code
> would be more valuable - that's Darrick, in this instance.
Eh, get_maintainers is usually fine. But adding something new in lib/
means more than fs people are going to be using/changing it, so get
their review early.
-Kees
--
Kees Cook
