| Message ID | 20190718125509.775525-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | iomap: hide iomap_sector with CONFIG_BLOCK=n | expand |
On Thu, Jul 18, 2019 at 02:55:01PM +0200, Arnd Bergmann wrote: > When CONFIG_BLOCK is disabled, SECTOR_SHIFT is unknown: > > In file included from <built-in>:3: > include/linux/iomap.h:76:48: error: use of undeclared identifier 'SECTOR_SHIFT' > return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; > > Since there are no callers in this case, just hide the function in > the same ifdef. > > Fixes: db074436f421 ("iomap: move the direct IO code into a separate file") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> Can we just not include iomap.c when CONFIG_BLOCK is not set? Which file do you see this with?
On Thu, Jul 18, 2019 at 2:57 PM Christoph Hellwig <hch@lst.de> wrote: > > On Thu, Jul 18, 2019 at 02:55:01PM +0200, Arnd Bergmann wrote: > > When CONFIG_BLOCK is disabled, SECTOR_SHIFT is unknown: > > > > In file included from <built-in>:3: > > include/linux/iomap.h:76:48: error: use of undeclared identifier 'SECTOR_SHIFT' > > return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; > > > > Since there are no callers in this case, just hide the function in > > the same ifdef. > > > > Fixes: db074436f421 ("iomap: move the direct IO code into a separate file") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > Can we just not include iomap.c when CONFIG_BLOCK is not set? > Which file do you see this with? The inclusion comes from the recently added header check in commit c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"). This just tries to include every header by itself to see if there are build failures from missing indirect includes. We probably don't want to add an exception for iomap.h there. Arnd
On Thu, Jul 18, 2019 at 03:03:15PM +0200, Arnd Bergmann wrote: > The inclusion comes from the recently added header check in commit > c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"). > > This just tries to include every header by itself to see if there are build > failures from missing indirect includes. We probably don't want to > add an exception for iomap.h there. I very much disagree with that check. We don't need to make every header compilable with a setup where it should not be included. That being said if you feel this is worth fixing I'd rather define SECTOR_SIZE/SECTOR_SHIFT unconditionally.
On Thu, Jul 18, 2019 at 03:08:35PM +0200, Christoph Hellwig wrote: > On Thu, Jul 18, 2019 at 03:03:15PM +0200, Arnd Bergmann wrote: > > The inclusion comes from the recently added header check in commit > > c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"). > > > > This just tries to include every header by itself to see if there are build > > failures from missing indirect includes. We probably don't want to > > add an exception for iomap.h there. > > I very much disagree with that check. We don't need to make every > header compilable with a setup where it should not be included. Seconded, unless there's some scenario where someone needs iomap when CONFIG_BLOCK=n (???) --D > That being said if you feel this is worth fixing I'd rather define > SECTOR_SIZE/SECTOR_SHIFT unconditionally.
On Thu, Jul 18, 2019 at 3:08 PM Christoph Hellwig <hch@lst.de> wrote: > > On Thu, Jul 18, 2019 at 03:03:15PM +0200, Arnd Bergmann wrote: > > The inclusion comes from the recently added header check in commit > > c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"). > > > > This just tries to include every header by itself to see if there are build > > failures from missing indirect includes. We probably don't want to > > add an exception for iomap.h there. > > I very much disagree with that check. We don't need to make every > header compilable with a setup where it should not be included. I do like the extra check there, and it did not seem to need too many fixes to get it working in the first place. > That being said if you feel this is worth fixing I'd rather define > SECTOR_SIZE/SECTOR_SHIFT unconditionally. I'll give that a try and send a replacement patch after build testing succeeds for a number of randconfig builds. Arnd
Hi. On Thu, Jul 18, 2019 at 11:28 PM Darrick J. Wong <darrick.wong@oracle.com> wrote: > > On Thu, Jul 18, 2019 at 03:08:35PM +0200, Christoph Hellwig wrote: > > On Thu, Jul 18, 2019 at 03:03:15PM +0200, Arnd Bergmann wrote: > > > The inclusion comes from the recently added header check in commit > > > c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"). > > > > > > This just tries to include every header by itself to see if there are build > > > failures from missing indirect includes. We probably don't want to > > > add an exception for iomap.h there. > > > > I very much disagree with that check. We don't need to make every > > header compilable with a setup where it should not be included. > > Seconded, unless there's some scenario where someone needs iomap when > CONFIG_BLOCK=n (???) I agree. There is no situation that iomap.h is included when CONFIG_BLOCK=n. So, it is pointless to surround offending code with #ifdef just for the purpose of satisfying the header-test. I started to think compiling all headers is more painful than useful. MW is closing, so I am thinking of disabling it for now to take time to re-think. diff --git a/init/Kconfig b/init/Kconfig index bd7d650d4a99..cbb31d134f7e 100644 --- a/init/Kconfig +++ b/init/Kconfig @@ -111,6 +111,7 @@ config HEADER_TEST config KERNEL_HEADER_TEST bool "Compile test kernel headers" depends on HEADER_TEST + depends on BROKEN help Headers in include/ are used to build external moduls. Compile test them to ensure they are self-contained, i.e. Maybe, we should compile-test headers only when it is reasonable to do so. -- Best Regards Masahiro Yamada
On 7/18/19 7:19 PM, Masahiro Yamada wrote: > Hi. > > On Thu, Jul 18, 2019 at 11:28 PM Darrick J. Wong > <darrick.wong@oracle.com> wrote: >> >> On Thu, Jul 18, 2019 at 03:08:35PM +0200, Christoph Hellwig wrote: >>> On Thu, Jul 18, 2019 at 03:03:15PM +0200, Arnd Bergmann wrote: >>>> The inclusion comes from the recently added header check in commit >>>> c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"). >>>> >>>> This just tries to include every header by itself to see if there are build >>>> failures from missing indirect includes. We probably don't want to >>>> add an exception for iomap.h there. >>> >>> I very much disagree with that check. We don't need to make every >>> header compilable with a setup where it should not be included. >> >> Seconded, unless there's some scenario where someone needs iomap when >> CONFIG_BLOCK=n (???) > > I agree. > > There is no situation that iomap.h is included when CONFIG_BLOCK=n. > So, it is pointless to surround offending code with #ifdef > just for the purpose of satisfying the header-test. > > > I started to think > compiling all headers is more painful than useful. > > > MW is closing, so I am thinking of disabling it for now > to take time to re-think. > > > diff --git a/init/Kconfig b/init/Kconfig > index bd7d650d4a99..cbb31d134f7e 100644 > --- a/init/Kconfig > +++ b/init/Kconfig > @@ -111,6 +111,7 @@ config HEADER_TEST > config KERNEL_HEADER_TEST > bool "Compile test kernel headers" > depends on HEADER_TEST > + depends on BROKEN > help > Headers in include/ are used to build external moduls. > Compile test them to ensure they are self-contained, i.e. > > > > Maybe, we should compile-test headers > only when it is reasonable to do so. Maybe. But I would find it easier to use if it were a make target instead of a Kconfig symbol, so someone could do $ make compile_test_headers for example. Then it would be done only on demand (or command). -- ~Randy
On Fri, Jul 19, 2019 at 11:24 AM Randy Dunlap <rdunlap@infradead.org> wrote: > > On 7/18/19 7:19 PM, Masahiro Yamada wrote: > > Hi. > > > > On Thu, Jul 18, 2019 at 11:28 PM Darrick J. Wong > > <darrick.wong@oracle.com> wrote: > >> > >> On Thu, Jul 18, 2019 at 03:08:35PM +0200, Christoph Hellwig wrote: > >>> On Thu, Jul 18, 2019 at 03:03:15PM +0200, Arnd Bergmann wrote: > >>>> The inclusion comes from the recently added header check in commit > >>>> c93a0368aaa2 ("kbuild: do not create wrappers for header-test-y"). > >>>> > >>>> This just tries to include every header by itself to see if there are build > >>>> failures from missing indirect includes. We probably don't want to > >>>> add an exception for iomap.h there. > >>> > >>> I very much disagree with that check. We don't need to make every > >>> header compilable with a setup where it should not be included. > >> > >> Seconded, unless there's some scenario where someone needs iomap when > >> CONFIG_BLOCK=n (???) > > > > I agree. > > > > There is no situation that iomap.h is included when CONFIG_BLOCK=n. > > So, it is pointless to surround offending code with #ifdef > > just for the purpose of satisfying the header-test. > > > > > > I started to think > > compiling all headers is more painful than useful. > > > > > > MW is closing, so I am thinking of disabling it for now > > to take time to re-think. > > > > > > diff --git a/init/Kconfig b/init/Kconfig > > index bd7d650d4a99..cbb31d134f7e 100644 > > --- a/init/Kconfig > > +++ b/init/Kconfig > > @@ -111,6 +111,7 @@ config HEADER_TEST > > config KERNEL_HEADER_TEST > > bool "Compile test kernel headers" > > depends on HEADER_TEST > > + depends on BROKEN > > help > > Headers in include/ are used to build external moduls. > > Compile test them to ensure they are self-contained, i.e. > > > > > > > > Maybe, we should compile-test headers > > only when it is reasonable to do so. > > Maybe. But I would find it easier to use if it were a make target > instead of a Kconfig symbol, so someone could do > $ make compile_test_headers You can do equivalent with this: $ ./scripts/config -e HEADER_TEST $ make include/ -- Best Regards Masahiro Yamada
On Fri, Jul 19, 2019 at 11:19:15AM +0900, Masahiro Yamada wrote: > I started to think > compiling all headers is more painful than useful. > > > MW is closing, so I am thinking of disabling it for now > to take time to re-think. For now this seems like the best idea. In the long run maybe we can limit the tests to certain configs, e.g. headers-$(CONFIG_IOMAP) += iomap.h in include/linux/Kbuild and base it off that?
On Fri, Jul 19, 2019 at 2:59 PM Christoph Hellwig <hch@infradead.org> wrote: > > On Fri, Jul 19, 2019 at 11:19:15AM +0900, Masahiro Yamada wrote: > > I started to think > > compiling all headers is more painful than useful. > > > > > > MW is closing, so I am thinking of disabling it for now > > to take time to re-think. > > For now this seems like the best idea. In the long run maybe we can > limit the tests to certain configs, e.g. > > > headers-$(CONFIG_IOMAP) += iomap.h I cannot find CONFIG_IOMAP. Do you mean header-test-$(CONFIG_BLOCK) += iomap.h ? > in include/linux/Kbuild > > and base it off that? Yes, I was thinking of that. -- Best Regards Masahiro Yamada
On Fri, Jul 19, 2019 at 03:16:55PM +0900, Masahiro Yamada wrote: > > headers-$(CONFIG_IOMAP) += iomap.h > > I cannot find CONFIG_IOMAP. > > Do you mean > > header-test-$(CONFIG_BLOCK) += iomap.h Yeah, we could use CONFIG_BLOCK.
diff --git a/include/linux/iomap.h b/include/linux/iomap.h index bc499ceae392..bb07f31e3b6f 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -70,11 +70,13 @@ struct iomap { const struct iomap_page_ops *page_ops; }; +#ifdef CONFIG_BLOCK static inline sector_t iomap_sector(struct iomap *iomap, loff_t pos) { return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; } +#endif /* * When a filesystem sets page_ops in an iomap mapping it returns, page_prepare
When CONFIG_BLOCK is disabled, SECTOR_SHIFT is unknown: In file included from <built-in>:3: include/linux/iomap.h:76:48: error: use of undeclared identifier 'SECTOR_SHIFT' return (iomap->addr + pos - iomap->offset) >> SECTOR_SHIFT; Since there are no callers in this case, just hide the function in the same ifdef. Fixes: db074436f421 ("iomap: move the direct IO code into a separate file") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- include/linux/iomap.h | 2 ++ 1 file changed, 2 insertions(+) -- 2.20.0