Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1531677 > unrolled thread

[PATCH] dax: try to avoid unused function warnings

Back to article view | Back to linux.kernel

Contents

  [PATCH] dax: try to avoid unused function warnings Arnd Bergmann <arnd@arndb.de> - 2016-11-28 22:20 +0100
    Re: [PATCH] dax: try to avoid unused function warnings Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-11-28 22:30 +0100
      Re: [PATCH] dax: try to avoid unused function warnings Dan Williams <dan.j.williams@intel.com> - 2016-11-28 23:20 +0100
        Re: [PATCH] dax: try to avoid unused function warnings Ross Zwisler <ross.zwisler@linux.intel.com> - 2016-11-29 00:00 +0100
          Re: [PATCH] dax: try to avoid unused function warnings Dan Williams <dan.j.williams@intel.com> - 2016-11-29 00:10 +0100
            Re: [PATCH] dax: try to avoid unused function warnings Dave Chinner <david@fromorbit.com> - 2016-11-29 03:20 +0100

#1531677 — [PATCH] dax: try to avoid unused function warnings

Without the get_block based I/O, we get warnings when CONFIG_FS_IOMAP
is disabled:

fs/dax.c:736:12: error: ‘dax_insert_mapping’ defined but not used [-Werror=unused-function]
fs/dax.c:512:12: error: ‘copy_user_dax’ defined but not used [-Werror=unused-function]
fs/dax.c:490:12: error: ‘dax_load_hole’ defined but not used [-Werror=unused-function]
fs/dax.c:294:14: error: ‘grab_mapping_entry’ defined but not used [-Werror=unused-function]

This patch blindly marks those as __maybe_unused, which avoids the warnings.
However, I suspect that there is actually more code in this file that should
not be provided without CONFIG_FS_IOMAP even though we don't get a warning
for it, and that we actually want a different rework, so please treat this
as a bug report. I have applied the patch locally in my randconfig build
setup to avoid seeing the warnings.

Fixes: 5ac65736f740 ("dax: rip out get_block based IO support")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/dax.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c
index b1fe228cd609..cf844e77b7b7 100644
--- a/fs/dax.c
+++ b/fs/dax.c
@@ -309,7 +309,7 @@ static void put_unlocked_mapping_entry(struct address_space *mapping,
  * persistent memory the benefit is doubtful. We can add that later if we can
  * show it helps.
  */
-static void *grab_mapping_entry(struct address_space *mapping, pgoff_t index,
+static __maybe_unused void * grab_mapping_entry(struct address_space *mapping, pgoff_t index,
 		unsigned long size_flag)
 {
 	bool pmd_downgrade = false; /* splitting 2MiB entry into 4k entries? */
@@ -489,7 +489,7 @@ int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index)
  * otherwise it will simply fall out of the page cache under memory
  * pressure without ever having been dirtied.
  */
-static int dax_load_hole(struct address_space *mapping, void *entry,
+static int __maybe_unused dax_load_hole(struct address_space *mapping, void *entry,
 			 struct vm_fault *vmf)
 {
 	struct page *page;
@@ -509,7 +509,7 @@ static int dax_load_hole(struct address_space *mapping, void *entry,
 	return VM_FAULT_LOCKED;
 }
 
-static int copy_user_dax(struct block_device *bdev, sector_t sector, size_t size,
+static int __maybe_unused copy_user_dax(struct block_device *bdev, sector_t sector, size_t size,
 		struct page *to, unsigned long vaddr)
 {
 	struct blk_dax_ctl dax = {
@@ -815,7 +815,7 @@ int dax_writeback_mapping_range(struct address_space *mapping,
 }
 EXPORT_SYMBOL_GPL(dax_writeback_mapping_range);
 
-static int dax_insert_mapping(struct address_space *mapping,
+static int __maybe_unused dax_insert_mapping(struct address_space *mapping,
 		struct block_device *bdev, sector_t sector, size_t size,
 		void **entryp, struct vm_area_struct *vma, struct vm_fault *vmf)
 {
-- 
2.9.0

[toc] | [next] | [standalone]

#1531681

On Mon, Nov 28, 2016 at 10:12:17PM +0100, Arnd Bergmann wrote:
> Without the get_block based I/O, we get warnings when CONFIG_FS_IOMAP
> is disabled:
> 
> fs/dax.c:736:12: error: ‘dax_insert_mapping’ defined but not used [-Werror=unused-function]
> fs/dax.c:512:12: error: ‘copy_user_dax’ defined but not used [-Werror=unused-function]
> fs/dax.c:490:12: error: ‘dax_load_hole’ defined but not used [-Werror=unused-function]
> fs/dax.c:294:14: error: ‘grab_mapping_entry’ defined but not used [-Werror=unused-function]
> 
> This patch blindly marks those as __maybe_unused, which avoids the warnings.
> However, I suspect that there is actually more code in this file that should
> not be provided without CONFIG_FS_IOMAP even though we don't get a warning
> for it, and that we actually want a different rework, so please treat this
> as a bug report. I have applied the patch locally in my randconfig build
> setup to avoid seeing the warnings.
> 
> Fixes: 5ac65736f740 ("dax: rip out get_block based IO support")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks for the report.  I think the right way to deal with this is to just
select FS_IOMAP when we pull in the DAX code.  I sent out a patch last week
that does this:

https://lkml.org/lkml/2016/11/23/591

[toc] | [prev] | [next] | [standalone]

#1531736

On Mon, Nov 28, 2016 at 1:24 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Mon, Nov 28, 2016 at 10:12:17PM +0100, Arnd Bergmann wrote:
>> Without the get_block based I/O, we get warnings when CONFIG_FS_IOMAP
>> is disabled:
>>
>> fs/dax.c:736:12: error: ‘dax_insert_mapping’ defined but not used [-Werror=unused-function]
>> fs/dax.c:512:12: error: ‘copy_user_dax’ defined but not used [-Werror=unused-function]
>> fs/dax.c:490:12: error: ‘dax_load_hole’ defined but not used [-Werror=unused-function]
>> fs/dax.c:294:14: error: ‘grab_mapping_entry’ defined but not used [-Werror=unused-function]
>>
>> This patch blindly marks those as __maybe_unused, which avoids the warnings.
>> However, I suspect that there is actually more code in this file that should
>> not be provided without CONFIG_FS_IOMAP even though we don't get a warning
>> for it, and that we actually want a different rework, so please treat this
>> as a bug report. I have applied the patch locally in my randconfig build
>> setup to avoid seeing the warnings.
>>
>> Fixes: 5ac65736f740 ("dax: rip out get_block based IO support")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Thanks for the report.  I think the right way to deal with this is to just
> select FS_IOMAP when we pull in the DAX code.  I sent out a patch last week
> that does this:
>
> https://lkml.org/lkml/2016/11/23/591

It seems awkward for both filesystems and the FS_DAX core to be
selecting FS_IOMAP.  In the end FS_DAX and FS_IOMAP are both libraries
of functionality that a filesystem can optionally use.  I think the
longer term FS_DAX stops being an independent user visible setting and
is instead selected by filesystems that want DAX.

[toc] | [prev] | [next] | [standalone]

#1531767

On Mon, Nov 28, 2016 at 02:13:29PM -0800, Dan Williams wrote:
> On Mon, Nov 28, 2016 at 1:24 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > On Mon, Nov 28, 2016 at 10:12:17PM +0100, Arnd Bergmann wrote:
> >> Without the get_block based I/O, we get warnings when CONFIG_FS_IOMAP
> >> is disabled:
> >>
> >> fs/dax.c:736:12: error: ‘dax_insert_mapping’ defined but not used [-Werror=unused-function]
> >> fs/dax.c:512:12: error: ‘copy_user_dax’ defined but not used [-Werror=unused-function]
> >> fs/dax.c:490:12: error: ‘dax_load_hole’ defined but not used [-Werror=unused-function]
> >> fs/dax.c:294:14: error: ‘grab_mapping_entry’ defined but not used [-Werror=unused-function]
> >>
> >> This patch blindly marks those as __maybe_unused, which avoids the warnings.
> >> However, I suspect that there is actually more code in this file that should
> >> not be provided without CONFIG_FS_IOMAP even though we don't get a warning
> >> for it, and that we actually want a different rework, so please treat this
> >> as a bug report. I have applied the patch locally in my randconfig build
> >> setup to avoid seeing the warnings.
> >>
> >> Fixes: 5ac65736f740 ("dax: rip out get_block based IO support")
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Thanks for the report.  I think the right way to deal with this is to just
> > select FS_IOMAP when we pull in the DAX code.  I sent out a patch last week
> > that does this:
> >
> > https://lkml.org/lkml/2016/11/23/591
> 
> It seems awkward for both filesystems and the FS_DAX core to be
> selecting FS_IOMAP.  In the end FS_DAX and FS_IOMAP are both libraries
> of functionality that a filesystem can optionally use.  I think the
> longer term FS_DAX stops being an independent user visible setting and
> is instead selected by filesystems that want DAX.

This doesn't make sense to me.  DAX is a user-selectable option that changes
behavior (at the user's request), but FS_IOMAP is a library of functionality
that is required for XFS and for DAX.

The filesystems can all work fine without DAX (hence the user option), but DAX
and XFS at least require FS_IOMAP to behave correctly.

If you made DAX a FS selectable option instead of a user selectable one, when
would a FS know it needs to include DAX support?

[toc] | [prev] | [next] | [standalone]

#1531772

On Mon, Nov 28, 2016 at 2:51 PM, Ross Zwisler
<ross.zwisler@linux.intel.com> wrote:
> On Mon, Nov 28, 2016 at 02:13:29PM -0800, Dan Williams wrote:
>> On Mon, Nov 28, 2016 at 1:24 PM, Ross Zwisler
>> <ross.zwisler@linux.intel.com> wrote:
>> > On Mon, Nov 28, 2016 at 10:12:17PM +0100, Arnd Bergmann wrote:
>> >> Without the get_block based I/O, we get warnings when CONFIG_FS_IOMAP
>> >> is disabled:
>> >>
>> >> fs/dax.c:736:12: error: ‘dax_insert_mapping’ defined but not used [-Werror=unused-function]
>> >> fs/dax.c:512:12: error: ‘copy_user_dax’ defined but not used [-Werror=unused-function]
>> >> fs/dax.c:490:12: error: ‘dax_load_hole’ defined but not used [-Werror=unused-function]
>> >> fs/dax.c:294:14: error: ‘grab_mapping_entry’ defined but not used [-Werror=unused-function]
>> >>
>> >> This patch blindly marks those as __maybe_unused, which avoids the warnings.
>> >> However, I suspect that there is actually more code in this file that should
>> >> not be provided without CONFIG_FS_IOMAP even though we don't get a warning
>> >> for it, and that we actually want a different rework, so please treat this
>> >> as a bug report. I have applied the patch locally in my randconfig build
>> >> setup to avoid seeing the warnings.
>> >>
>> >> Fixes: 5ac65736f740 ("dax: rip out get_block based IO support")
>> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >
>> > Thanks for the report.  I think the right way to deal with this is to just
>> > select FS_IOMAP when we pull in the DAX code.  I sent out a patch last week
>> > that does this:
>> >
>> > https://lkml.org/lkml/2016/11/23/591
>>
>> It seems awkward for both filesystems and the FS_DAX core to be
>> selecting FS_IOMAP.  In the end FS_DAX and FS_IOMAP are both libraries
>> of functionality that a filesystem can optionally use.  I think the
>> longer term FS_DAX stops being an independent user visible setting and
>> is instead selected by filesystems that want DAX.
>
> This doesn't make sense to me.  DAX is a user-selectable option that changes
> behavior (at the user's request), but FS_IOMAP is a library of functionality
> that is required for XFS and for DAX.
>
> The filesystems can all work fine without DAX (hence the user option), but DAX
> and XFS at least require FS_IOMAP to behave correctly.
>
> If you made DAX a FS selectable option instead of a user selectable one, when
> would a FS know it needs to include DAX support?

With a user-selectable DAX knob per-filesystem, XFS_DAX, EXT4_DAX, etc...

[toc] | [prev] | [next] | [standalone]

#1531865

On Mon, Nov 28, 2016 at 03:08:18PM -0800, Dan Williams wrote:
> On Mon, Nov 28, 2016 at 2:51 PM, Ross Zwisler
> <ross.zwisler@linux.intel.com> wrote:
> > On Mon, Nov 28, 2016 at 02:13:29PM -0800, Dan Williams wrote:
> >> On Mon, Nov 28, 2016 at 1:24 PM, Ross Zwisler
> >> <ross.zwisler@linux.intel.com> wrote:
> >> > On Mon, Nov 28, 2016 at 10:12:17PM +0100, Arnd Bergmann wrote:
> >> >> Without the get_block based I/O, we get warnings when CONFIG_FS_IOMAP
> >> >> is disabled:
> >> >>
> >> >> fs/dax.c:736:12: error: ‘dax_insert_mapping’ defined but not used [-Werror=unused-function]
> >> >> fs/dax.c:512:12: error: ‘copy_user_dax’ defined but not used [-Werror=unused-function]
> >> >> fs/dax.c:490:12: error: ‘dax_load_hole’ defined but not used [-Werror=unused-function]
> >> >> fs/dax.c:294:14: error: ‘grab_mapping_entry’ defined but not used [-Werror=unused-function]
> >> >>
> >> >> This patch blindly marks those as __maybe_unused, which avoids the warnings.
> >> >> However, I suspect that there is actually more code in this file that should
> >> >> not be provided without CONFIG_FS_IOMAP even though we don't get a warning
> >> >> for it, and that we actually want a different rework, so please treat this
> >> >> as a bug report. I have applied the patch locally in my randconfig build
> >> >> setup to avoid seeing the warnings.
> >> >>
> >> >> Fixes: 5ac65736f740 ("dax: rip out get_block based IO support")
> >> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >> >
> >> > Thanks for the report.  I think the right way to deal with this is to just
> >> > select FS_IOMAP when we pull in the DAX code.  I sent out a patch last week
> >> > that does this:
> >> >
> >> > https://lkml.org/lkml/2016/11/23/591
> >>
> >> It seems awkward for both filesystems and the FS_DAX core to be
> >> selecting FS_IOMAP.  In the end FS_DAX and FS_IOMAP are both libraries
> >> of functionality that a filesystem can optionally use.  I think the
> >> longer term FS_DAX stops being an independent user visible setting and
> >> is instead selected by filesystems that want DAX.
> >
> > This doesn't make sense to me.  DAX is a user-selectable option that changes
> > behavior (at the user's request), but FS_IOMAP is a library of functionality
> > that is required for XFS and for DAX.
> >
> > The filesystems can all work fine without DAX (hence the user option), but DAX
> > and XFS at least require FS_IOMAP to behave correctly.
> >
> > If you made DAX a FS selectable option instead of a user selectable one, when
> > would a FS know it needs to include DAX support?
> 
> With a user-selectable DAX knob per-filesystem, XFS_DAX, EXT4_DAX, etc...

That's just silly. Requiring users to configure every filesystem
that can support DAX to support DAX at config time is unneeded
config space bloat. DAX has an iomap config dependency, so just
select it when DAX is selected - everything else should just be
automatically and nobody else needs to care what build dependencies
DAX has.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web