| Message ID | 20161128211233.2446605-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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? -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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... -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
On Monday, November 28, 2016 2:24:00 PM CET Ross Zwisler 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 we never got agreement on the approach, and we still get the warnings above in v4.10. Should we use my patch to fix up 4.10 and get a clean build again? It no longer applies, but I have a rebased version that I can send. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 10, 2017 at 05:15:57PM +0100, Arnd Bergmann wrote: > On Monday, November 28, 2016 2:24:00 PM CET Ross Zwisler 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 we never got agreement on the approach, and we still get the > warnings above in v4.10. Should we use my patch to fix up 4.10 and get > a clean build again? It no longer applies, but I have a rebased version > that I can send. > > Arnd Thanks for the reminder, this one slipped through the cracks. I still think the right way to go with this is to fix up DAX's dependence on FS_IOMAP. I'll send out an updated patch. -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) {
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(-)