[PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Arnd Bergmann]

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(+)

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
-- 
2.20.0

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Christoph Hellwig]

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?

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Arnd Bergmann]

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

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Christoph Hellwig]

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.

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Darrick J. Wong]

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.

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Masahiro Yamada]

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

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Randy Dunlap]

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

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Masahiro Yamada]

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

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Christoph Hellwig]

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?

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Masahiro Yamada]

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

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Christoph Hellwig]

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.

Re: [PATCH] iomap: hide iomap_sector with CONFIG_BLOCK=n

[Arnd Bergmann]

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