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

Groups > linux.kernel > #1641696 > unrolled thread

[PATCH 1/2] dax: hide block device code in #ifdef

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] dax: hide block device code in #ifdef Arnd Bergmann <arnd@arndb.de> - 2017-05-15 15:50 +0200
    [PATCH 2/2] fs: xfs: add DAX dependency Arnd Bergmann <arnd@arndb.de> - 2017-05-15 15:50 +0200
    Re: [PATCH 1/2] dax: hide block device code in #ifdef Dan Williams <dan.j.williams@intel.com> - 2017-05-15 16:20 +0200
      Re: [PATCH 1/2] dax: hide block device code in #ifdef Arnd Bergmann <arnd@arndb.de> - 2017-05-15 16:30 +0200
        Re: [PATCH 1/2] dax: hide block device code in #ifdef "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-05-15 18:00 +0200
          Re: [PATCH 1/2] dax: hide block device code in #ifdef Arnd Bergmann <arnd@arndb.de> - 2017-05-16 13:20 +0200

#1641696 — [PATCH 1/2] dax: hide block device code in #ifdef

We allow configurations with CONFIG_BLOCK=n and CONFIG_DAX=y, which now
results in a link error:

drivers/dax/super.c: In function 'bdev_dax_pgoff':
drivers/dax/super.c:50:26: error: implicit declaration of function 'get_start_sect'; did you mean 'get_task_cred'? [-Werror=implicit-function-declaration]

The two obvious ways to avoid the link error are to either add an #ifdef
around the code that was moved from fs/block_dev.c, or to disallow the
configuration. I could not see if there is or is not a reason to support
this combination of options, but in case there is, the #ifdef is the
safer choice.

Fixes: ef51042472f5 ("block, dax: move "select DAX" from BLOCK to FS_DAX")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/dax/super.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dax/super.c b/drivers/dax/super.c
index ebf43f531ada..6ed32aac8bbe 100644
--- a/drivers/dax/super.c
+++ b/drivers/dax/super.c
@@ -44,6 +44,7 @@ void dax_read_unlock(int id)
 }
 EXPORT_SYMBOL_GPL(dax_read_unlock);
 
+#ifdef CONFIG_BLOCK
 int bdev_dax_pgoff(struct block_device *bdev, sector_t sector, size_t size,
 		pgoff_t *pgoff)
 {
@@ -112,6 +113,7 @@ int __bdev_dax_supported(struct super_block *sb, int blocksize)
 	return 0;
 }
 EXPORT_SYMBOL_GPL(__bdev_dax_supported);
+#endif
 
 /**
  * struct dax_device - anchor object for dax services
-- 
2.9.0

[toc] | [next] | [standalone]

#1641704 — [PATCH 2/2] fs: xfs: add DAX dependency

We can now build xfs while CONFIG_DAX is set to 'm', resulting in a
link error when xfs is built-in:

fs/xfs/xfs_iomap.o: In function `xfs_file_iomap_end':
fs/xfs/xfs_iomap.c:1152: undefined reference to `put_dax'
fs/xfs/xfs_iomap.o: In function `xfs_file_iomap_begin':
fs/xfs/xfs_iomap.c:1071: undefined reference to `dax_get_by_host'

Previously, XFS could only be built with CONFIG_DAX=y because of
the CONFIG_BLOCK dependency. The other users of dax handle this
differently:

- In ext2 and ext4, there are checks for CONFIG_FS_DAX around every
  API usage, and CONFIG_FS_DAX is only enabled when CONFIG_DAX is
  built-in.

- CONFIG_BLK_DEV_MD implies 'select DAX', so it's always enabled
  there as well.

This changes XFS to do the same as ext2/4 and only call into the
API conditionally.

Fixes: ef51042472f5 ("block, dax: move "select DAX" from BLOCK to FS_DAX")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 fs/xfs/xfs_iomap.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index a63f61c256bd..b732ba6f892c 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1067,7 +1067,7 @@ xfs_file_iomap_begin(
 
 	/* optionally associate a dax device with the iomap bdev */
 	bdev = iomap->bdev;
-	if (blk_queue_dax(bdev->bd_queue))
+	if (IS_ENABLED(CONFIG_FS_DAX) && blk_queue_dax(bdev->bd_queue))
 		iomap->dax_dev = dax_get_by_host(bdev->bd_disk->disk_name);
 	else
 		iomap->dax_dev = NULL;
@@ -1149,7 +1149,9 @@ xfs_file_iomap_end(
 	unsigned		flags,
 	struct iomap		*iomap)
 {
-	put_dax(iomap->dax_dev);
+	if (IS_ENABLED(CONFIG_FS_DAX))
+		put_dax(iomap->dax_dev);
+
 	if ((flags & IOMAP_WRITE) && iomap->type == IOMAP_DELALLOC)
 		return xfs_file_iomap_end_delalloc(XFS_I(inode), offset,
 				length, written, iomap);
-- 
2.9.0

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

#1641728

On Mon, May 15, 2017 at 6:44 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> We allow configurations with CONFIG_BLOCK=n and CONFIG_DAX=y, which now
> results in a link error:
>
> drivers/dax/super.c: In function 'bdev_dax_pgoff':
> drivers/dax/super.c:50:26: error: implicit declaration of function 'get_start_sect'; did you mean 'get_task_cred'? [-Werror=implicit-function-declaration]
>
> The two obvious ways to avoid the link error are to either add an #ifdef
> around the code that was moved from fs/block_dev.c, or to disallow the
> configuration. I could not see if there is or is not a reason to support
> this combination of options, but in case there is, the #ifdef is the
> safer choice.
>
> Fixes: ef51042472f5 ("block, dax: move "select DAX" from BLOCK to FS_DAX")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Yup, the same one I came up with:

https://patchwork.kernel.org/patch/9725513/

If you have a a pull request coming up soon you can add my Acked-by to
both of these patches, otherwise I'll send what I have along at the
end of the week.

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

#1641739

On Mon, May 15, 2017 at 4:11 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> On Mon, May 15, 2017 at 6:44 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> We allow configurations with CONFIG_BLOCK=n and CONFIG_DAX=y, which now
>> results in a link error:
>>
>> drivers/dax/super.c: In function 'bdev_dax_pgoff':
>> drivers/dax/super.c:50:26: error: implicit declaration of function 'get_start_sect'; did you mean 'get_task_cred'? [-Werror=implicit-function-declaration]
>>
>> The two obvious ways to avoid the link error are to either add an #ifdef
>> around the code that was moved from fs/block_dev.c, or to disallow the
>> configuration. I could not see if there is or is not a reason to support
>> this combination of options, but in case there is, the #ifdef is the
>> safer choice.
>>
>> Fixes: ef51042472f5 ("block, dax: move "select DAX" from BLOCK to FS_DAX")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>
> Yup, the same one I came up with:
>
> https://patchwork.kernel.org/patch/9725513/
>
> If you have a a pull request coming up soon you can add my Acked-by to
> both of these patches, otherwise I'll send what I have along at the
> end of the week.

I didn't plan to send a pull request, please use whatever you have.

      Arnd

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

#1641849

On Mon, May 15, 2017 at 04:20:09PM +0200, Arnd Bergmann wrote:
> On Mon, May 15, 2017 at 4:11 PM, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Mon, May 15, 2017 at 6:44 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> We allow configurations with CONFIG_BLOCK=n and CONFIG_DAX=y, which now
> >> results in a link error:
> >>
> >> drivers/dax/super.c: In function 'bdev_dax_pgoff':
> >> drivers/dax/super.c:50:26: error: implicit declaration of function 'get_start_sect'; did you mean 'get_task_cred'? [-Werror=implicit-function-declaration]
> >>
> >> The two obvious ways to avoid the link error are to either add an #ifdef
> >> around the code that was moved from fs/block_dev.c, or to disallow the
> >> configuration. I could not see if there is or is not a reason to support
> >> this combination of options, but in case there is, the #ifdef is the
> >> safer choice.
> >>
> >> Fixes: ef51042472f5 ("block, dax: move "select DAX" from BLOCK to FS_DAX")
> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >
> > Yup, the same one I came up with:
> >
> > https://patchwork.kernel.org/patch/9725513/
> >
> > If you have a a pull request coming up soon you can add my Acked-by to
> > both of these patches, otherwise I'll send what I have along at the
> > end of the week.
> 
> I didn't plan to send a pull request, please use whatever you have.

I don't mind pulling the (second) patch in via the xfs tree.

Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

--D

> 
>       Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1642418

On Mon, May 15, 2017 at 5:50 PM, Darrick J. Wong
<darrick.wong@oracle.com> wrote:
> On Mon, May 15, 2017 at 04:20:09PM +0200, Arnd Bergmann wrote:
>> On Mon, May 15, 2017 at 4:11 PM, Dan Williams <dan.j.williams@intel.com> wrote:
>> > On Mon, May 15, 2017 at 6:44 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> >> We allow configurations with CONFIG_BLOCK=n and CONFIG_DAX=y, which now
>> >> results in a link error:
>> >>
>> >> drivers/dax/super.c: In function 'bdev_dax_pgoff':
>> >> drivers/dax/super.c:50:26: error: implicit declaration of function 'get_start_sect'; did you mean 'get_task_cred'? [-Werror=implicit-function-declaration]
>> >>
>> >> The two obvious ways to avoid the link error are to either add an #ifdef
>> >> around the code that was moved from fs/block_dev.c, or to disallow the
>> >> configuration. I could not see if there is or is not a reason to support
>> >> this combination of options, but in case there is, the #ifdef is the
>> >> safer choice.
>> >>
>> >> Fixes: ef51042472f5 ("block, dax: move "select DAX" from BLOCK to FS_DAX")
>> >> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> >
>> > Yup, the same one I came up with:
>> >
>> > https://patchwork.kernel.org/patch/9725513/
>> >
>> > If you have a a pull request coming up soon you can add my Acked-by to
>> > both of these patches, otherwise I'll send what I have along at the
>> > end of the week.
>>
>> I didn't plan to send a pull request, please use whatever you have.
>
> I don't mind pulling the (second) patch in via the xfs tree.
>
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
>

I think Dan's "dax, xfs, ext4: compile out iomap-dax paths in the
FS_DAX=n case" handles this in a nicer way, so my patch 2/2 is not
needed any more.

      Arnd

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web