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.
--
Cheers,
Stephen Rothwell
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.
And, as mentioned before, checkpatch.pl absolutely screams about this
commit. Lots of code style issues (unnamed arguments in declarations,
variables defined in the middle of function bodies, etc). Please adjust
these things so it's easier for other maintainers to work with and on
this code. :)
-Kees
--
Kees Cook
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.
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.
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.
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.
>
> 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.
>
> 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.
Heh, I hadn't realized that the twf lift had already gone to -next.
I just sent a bunch more fixes:
https://lore.kernel.org/linux-bcachefs/170785143225.984170.828418190611017982.stg-ugh@frogsfrogsfrogs/T/#u
Though I guess in the haze of ~700 patches I forgot that I'd added the
module annotations to one of Kent's patches:
https://git.kernel.org/pub/scm/linux/kernel/git/djwong/xfs-linux.git/commit/?h=twf-cleanups&id=d980d2d451debda0e52868382fee32273e2946ae
--D
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