Build failures due to commit 416161db (btrfs: offline dedupe)

Build failures due to commit 416161db (btrfs: offline dedupe)

[Guenter Roeck]

fs/btrfs/ioctl.c: In function 'btrfs_ioctl_file_extent_same':
fs/btrfs/ioctl.c:2802:3: error: implicit declaration of function '__put_user_unaligned' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
make[2]: *** [fs/btrfs/ioctl.o] Error 1
make[2]: *** Waiting for unfinished jobs....

Seen with alpha:allmodconfig, arm:allmodconfig, m68k:allmodconfig, and
xtensa:allmodconfig.

Guenter

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Geert Uytterhoeven]

On Fri, Sep 13, 2013 at 3:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> fs/btrfs/ioctl.c: In function 'btrfs_ioctl_file_extent_same':
> fs/btrfs/ioctl.c:2802:3: error: implicit declaration of function '__put_user_unaligned' [-Werror=implicit-function-declaration]
> cc1: some warnings being treated as errors
> make[2]: *** [fs/btrfs/ioctl.o] Error 1
> make[2]: *** Waiting for unfinished jobs....
>
> Seen with alpha:allmodconfig, arm:allmodconfig, m68k:allmodconfig, and
> xtensa:allmodconfig.

Known issue, cfr. my early warning 10 days ago:

"Btrfs is the first user of __put_user_unaligned() outside the compat code,
hence now all 32-bit architectures should make sure to implement this, too."

http://marc.info/?l=linux-arch&m=137820065929216&w=2

and today's thread https://lkml.org/lkml/2013/9/12/814

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Guenter Roeck]

On Fri, Sep 13, 2013 at 03:52:43PM +0200, Geert Uytterhoeven wrote:
> On Fri, Sep 13, 2013 at 3:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > fs/btrfs/ioctl.c: In function 'btrfs_ioctl_file_extent_same':
> > fs/btrfs/ioctl.c:2802:3: error: implicit declaration of function '__put_user_unaligned' [-Werror=implicit-function-declaration]
> > cc1: some warnings being treated as errors
> > make[2]: *** [fs/btrfs/ioctl.o] Error 1
> > make[2]: *** Waiting for unfinished jobs....
> >
> > Seen with alpha:allmodconfig, arm:allmodconfig, m68k:allmodconfig, and
> > xtensa:allmodconfig.
> 
> Known issue, cfr. my early warning 10 days ago:
> 
> "Btrfs is the first user of __put_user_unaligned() outside the compat code,
> hence now all 32-bit architectures should make sure to implement this, too."
> 
> http://marc.info/?l=linux-arch&m=137820065929216&w=2
> 
> and today's thread https://lkml.org/lkml/2013/9/12/814
> 

It doesn't seem right that a patch breaks the build for several platforms, and
the problem is then blamed on the platform code instead of the code that is
introducing the problem.

Maybe we should add BROKEN to the btrfs dependencies for the affected platforms.
After all, it _is_ broken.

Guenter

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Chris Mason]

Quoting Guenter Roeck (2013-09-13 12:35:35)
> On Fri, Sep 13, 2013 at 03:52:43PM +0200, Geert Uytterhoeven wrote:
> > On Fri, Sep 13, 2013 at 3:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > > fs/btrfs/ioctl.c: In function 'btrfs_ioctl_file_extent_same':
> > > fs/btrfs/ioctl.c:2802:3: error: implicit declaration of function '__put_user_unaligned' [-Werror=implicit-function-declaration]
> > > cc1: some warnings being treated as errors
> > > make[2]: *** [fs/btrfs/ioctl.o] Error 1
> > > make[2]: *** Waiting for unfinished jobs....
> > >
> > > Seen with alpha:allmodconfig, arm:allmodconfig, m68k:allmodconfig, and
> > > xtensa:allmodconfig.
> > 
> > Known issue, cfr. my early warning 10 days ago:
> > 
> > "Btrfs is the first user of __put_user_unaligned() outside the compat code,
> > hence now all 32-bit architectures should make sure to implement this, too."
> > 
> > http://marc.info/?l=linux-arch&m=137820065929216&w=2
> > 
> > and today's thread https://lkml.org/lkml/2013/9/12/814
> > 
> 
> It doesn't seem right that a patch breaks the build for several platforms, and
> the problem is then blamed on the platform code instead of the code that is
> introducing the problem.
> 
> Maybe we should add BROKEN to the btrfs dependencies for the affected platforms.
> After all, it _is_ broken.

I'm happy to fix this with a bigger put of the info struct, just
let me know the preferred arch-happy solution.

-chris

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Guenter Roeck]

On Fri, Sep 13, 2013 at 01:00:22PM -0400, Chris Mason wrote:
> Quoting Guenter Roeck (2013-09-13 12:35:35)
> > On Fri, Sep 13, 2013 at 03:52:43PM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Sep 13, 2013 at 3:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > > > fs/btrfs/ioctl.c: In function 'btrfs_ioctl_file_extent_same':
> > > > fs/btrfs/ioctl.c:2802:3: error: implicit declaration of function '__put_user_unaligned' [-Werror=implicit-function-declaration]
> > > > cc1: some warnings being treated as errors
> > > > make[2]: *** [fs/btrfs/ioctl.o] Error 1
> > > > make[2]: *** Waiting for unfinished jobs....
> > > >
> > > > Seen with alpha:allmodconfig, arm:allmodconfig, m68k:allmodconfig, and
> > > > xtensa:allmodconfig.
> > > 
> > > Known issue, cfr. my early warning 10 days ago:
> > > 
> > > "Btrfs is the first user of __put_user_unaligned() outside the compat code,
> > > hence now all 32-bit architectures should make sure to implement this, too."
> > > 
> > > http://marc.info/?l=linux-arch&m=137820065929216&w=2
> > > 
> > > and today's thread https://lkml.org/lkml/2013/9/12/814
> > > 
> > 
> > It doesn't seem right that a patch breaks the build for several platforms, and
> > the problem is then blamed on the platform code instead of the code that is
> > introducing the problem.
> > 
> > Maybe we should add BROKEN to the btrfs dependencies for the affected platforms.
> > After all, it _is_ broken.
> 
> I'm happy to fix this with a bigger put of the info struct, just
> let me know the preferred arch-happy solution.
> 
Me not either. The only requirement I would have is that it should not break
a build. Of course, it would be even better if it would actually work ;-).

Guenter

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Mark Fasheh]

On Fri, Sep 13, 2013 at 01:00:22PM -0400, Chris Mason wrote:
> Quoting Guenter Roeck (2013-09-13 12:35:35)
> > On Fri, Sep 13, 2013 at 03:52:43PM +0200, Geert Uytterhoeven wrote:
> > > On Fri, Sep 13, 2013 at 3:33 PM, Guenter Roeck <linux@roeck-us.net> wrote:
> > > > fs/btrfs/ioctl.c: In function 'btrfs_ioctl_file_extent_same':
> > > > fs/btrfs/ioctl.c:2802:3: error: implicit declaration of function '__put_user_unaligned' [-Werror=implicit-function-declaration]
> > > > cc1: some warnings being treated as errors
> > > > make[2]: *** [fs/btrfs/ioctl.o] Error 1
> > > > make[2]: *** Waiting for unfinished jobs....
> > > >
> > > > Seen with alpha:allmodconfig, arm:allmodconfig, m68k:allmodconfig, and
> > > > xtensa:allmodconfig.
> > > 
> > > Known issue, cfr. my early warning 10 days ago:
> > > 
> > > "Btrfs is the first user of __put_user_unaligned() outside the compat code,
> > > hence now all 32-bit architectures should make sure to implement this, too."
> > > 
> > > http://marc.info/?l=linux-arch&m=137820065929216&w=2
> > > 
> > > and today's thread https://lkml.org/lkml/2013/9/12/814
> > > 
> > 
> > It doesn't seem right that a patch breaks the build for several platforms, and
> > the problem is then blamed on the platform code instead of the code that is
> > introducing the problem.
> > 
> > Maybe we should add BROKEN to the btrfs dependencies for the affected platforms.
> > After all, it _is_ broken.
> 
> I'm happy to fix this with a bigger put of the info struct, just
> let me know the preferred arch-happy solution.

In fact old versions of the patch were putting the whole struct but during
review I was asked to change it. This should be very straight forward to fix
so long as we all stay calm ;)
	--Mark

--
Mark Fasheh

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Chris Mason]

Quoting Mark Fasheh (2013-09-13 13:58:01)
> On Fri, Sep 13, 2013 at 01:00:22PM -0400, Chris Mason wrote:
> > Quoting Guenter Roeck (2013-09-13 12:35:35)
> > I'm happy to fix this with a bigger put of the info struct, just
> > let me know the preferred arch-happy solution.
> 
> In fact old versions of the patch were putting the whole struct but during
> review I was asked to change it. This should be very straight forward to fix
> so long as we all stay calm ;)
>         --Mark

Mark, could you please send a patch for the whole-struct option until
the unaligned put is upstreamed?

-chris

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Mark Fasheh]

On Fri, Sep 13, 2013 at 03:33:34PM -0400, Chris Mason wrote:
> Mark, could you please send a patch for the whole-struct option until
> the unaligned put is upstreamed?
> 
> -chris

Here you go. It's been lightly tested and needs review.

Thanks,
	--Mark

--
Mark Fasheh

From: Mark Fasheh <mfasheh@suse.de>

[PATCH] btrfs: change extent-same to copy entire argument struct

btrfs_ioctl_file_extent_same() uses __put_user_unaligned() to copy some data
back to it's argument struct. Unfortunately, not all architectures provide
__put_user_unaligned(), so compiles break on them if btrfs is selected.

Instead, just copy the whole struct in / out at the start and end of
operations, respectively.

Signed-off-by: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/ioctl.c | 76 +++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 45 insertions(+), 31 deletions(-)

diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 1a5b946..25d6920 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -2696,9 +2696,9 @@ out_unlock:
 static long btrfs_ioctl_file_extent_same(struct file *file,
 					 void __user *argp)
 {
-	struct btrfs_ioctl_same_args *args = argp;
-	struct btrfs_ioctl_same_args same;
-	struct btrfs_ioctl_same_extent_info info;
+	struct btrfs_ioctl_same_args tmp;
+	struct btrfs_ioctl_same_args *same;
+	struct btrfs_ioctl_same_extent_info *info;
 	struct inode *src = file->f_dentry->d_inode;
 	struct file *dst_file = NULL;
 	struct inode *dst;
@@ -2706,6 +2706,7 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
 	u64 len;
 	int i;
 	int ret;
+	unsigned long size;
 	u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
 	bool is_admin = capable(CAP_SYS_ADMIN);
 
@@ -2716,15 +2717,30 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
 	if (ret)
 		return ret;
 
-	if (copy_from_user(&same,
+	if (copy_from_user(&tmp,
 			   (struct btrfs_ioctl_same_args __user *)argp,
-			   sizeof(same))) {
+			   sizeof(tmp))) {
 		ret = -EFAULT;
 		goto out;
 	}
 
-	off = same.logical_offset;
-	len = same.length;
+	size = sizeof(tmp) +
+		tmp.dest_count * sizeof(struct btrfs_ioctl_same_extent_info);
+
+	same = kmalloc(size, GFP_NOFS);
+	if (!same) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	if (copy_from_user(same,
+			   (struct btrfs_ioctl_same_args __user *)argp, size)) {
+		ret = -EFAULT;
+		goto out;
+	}
+
+	off = same->logical_offset;
+	len = same->length;
 
 	/*
 	 * Limit the total length we will dedupe for each operation.
@@ -2752,27 +2768,28 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
 	if (!S_ISREG(src->i_mode))
 		goto out;
 
-	ret = 0;
-	for (i = 0; i < same.dest_count; i++) {
-		if (copy_from_user(&info, &args->info[i], sizeof(info))) {
-			ret = -EFAULT;
-			goto out;
-		}
+	/* pre-format output fields to sane values */
+	for (i = 0; i < same->dest_count; i++) {
+		same->info[i].bytes_deduped = 0ULL;
+		same->info[i].status = 0;
+	}
 
-		info.bytes_deduped = 0;
+	ret = 0;
+	for (i = 0; i < same->dest_count; i++) {
+		info = &same->info[i];
 
-		dst_file = fget(info.fd);
+		dst_file = fget(info->fd);
 		if (!dst_file) {
-			info.status = -EBADF;
+			info->status = -EBADF;
 			goto next;
 		}
 
 		if (!(is_admin || (dst_file->f_mode & FMODE_WRITE))) {
-			info.status = -EINVAL;
+			info->status = -EINVAL;
 			goto next;
 		}
 
-		info.status = -EXDEV;
+		info->status = -EXDEV;
 		if (file->f_path.mnt != dst_file->f_path.mnt)
 			goto next;
 
@@ -2781,32 +2798,29 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
 			goto next;
 
 		if (S_ISDIR(dst->i_mode)) {
-			info.status = -EISDIR;
+			info->status = -EISDIR;
 			goto next;
 		}
 
 		if (!S_ISREG(dst->i_mode)) {
-			info.status = -EACCES;
+			info->status = -EACCES;
 			goto next;
 		}
 
-		info.status = btrfs_extent_same(src, off, len, dst,
-						info.logical_offset);
-		if (info.status == 0)
-			info.bytes_deduped += len;
+		info->status = btrfs_extent_same(src, off, len, dst,
+						info->logical_offset);
+		if (info->status == 0)
+			info->bytes_deduped += len;
 
 next:
 		if (dst_file)
 			fput(dst_file);
-
-		if (__put_user_unaligned(info.status, &args->info[i].status) ||
-		    __put_user_unaligned(info.bytes_deduped,
-					 &args->info[i].bytes_deduped)) {
-			ret = -EFAULT;
-			goto out;
-		}                                                               
 	}
 
+	ret = copy_to_user(argp, same, size);
+	if (ret)
+		ret = -EFAULT;
+
 out:
 	mnt_drop_write_file(file);
 	return ret;
-- 
1.8.1.4

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Guenter Roeck]

On Tue, Sep 17, 2013 at 03:43:54PM -0700, Mark Fasheh wrote:
> On Fri, Sep 13, 2013 at 03:33:34PM -0400, Chris Mason wrote:
> > Mark, could you please send a patch for the whole-struct option until
> > the unaligned put is upstreamed?
> > 
> > -chris
> 
> Here you go. It's been lightly tested and needs review.
> 
At the very least it does fix the build error on the affected platforms.

Guenter

> Thanks,
> 	--Mark
> 
> --
> Mark Fasheh
> 
> From: Mark Fasheh <mfasheh@suse.de>
> 
> [PATCH] btrfs: change extent-same to copy entire argument struct
> 
> btrfs_ioctl_file_extent_same() uses __put_user_unaligned() to copy some data
> back to it's argument struct. Unfortunately, not all architectures provide
> __put_user_unaligned(), so compiles break on them if btrfs is selected.
> 
> Instead, just copy the whole struct in / out at the start and end of
> operations, respectively.
> 
> Signed-off-by: Mark Fasheh <mfasheh@suse.de>
> ---
>  fs/btrfs/ioctl.c | 76 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
> index 1a5b946..25d6920 100644
> --- a/fs/btrfs/ioctl.c
> +++ b/fs/btrfs/ioctl.c
> @@ -2696,9 +2696,9 @@ out_unlock:
>  static long btrfs_ioctl_file_extent_same(struct file *file,
>  					 void __user *argp)
>  {
> -	struct btrfs_ioctl_same_args *args = argp;
> -	struct btrfs_ioctl_same_args same;
> -	struct btrfs_ioctl_same_extent_info info;
> +	struct btrfs_ioctl_same_args tmp;
> +	struct btrfs_ioctl_same_args *same;
> +	struct btrfs_ioctl_same_extent_info *info;
>  	struct inode *src = file->f_dentry->d_inode;
>  	struct file *dst_file = NULL;
>  	struct inode *dst;
> @@ -2706,6 +2706,7 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
>  	u64 len;
>  	int i;
>  	int ret;
> +	unsigned long size;
>  	u64 bs = BTRFS_I(src)->root->fs_info->sb->s_blocksize;
>  	bool is_admin = capable(CAP_SYS_ADMIN);
>  
> @@ -2716,15 +2717,30 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
>  	if (ret)
>  		return ret;
>  
> -	if (copy_from_user(&same,
> +	if (copy_from_user(&tmp,
>  			   (struct btrfs_ioctl_same_args __user *)argp,
> -			   sizeof(same))) {
> +			   sizeof(tmp))) {
>  		ret = -EFAULT;
>  		goto out;
>  	}
>  
> -	off = same.logical_offset;
> -	len = same.length;
> +	size = sizeof(tmp) +
> +		tmp.dest_count * sizeof(struct btrfs_ioctl_same_extent_info);
> +
> +	same = kmalloc(size, GFP_NOFS);
> +	if (!same) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	if (copy_from_user(same,
> +			   (struct btrfs_ioctl_same_args __user *)argp, size)) {
> +		ret = -EFAULT;
> +		goto out;
> +	}
> +
> +	off = same->logical_offset;
> +	len = same->length;
>  
>  	/*
>  	 * Limit the total length we will dedupe for each operation.
> @@ -2752,27 +2768,28 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
>  	if (!S_ISREG(src->i_mode))
>  		goto out;
>  
> -	ret = 0;
> -	for (i = 0; i < same.dest_count; i++) {
> -		if (copy_from_user(&info, &args->info[i], sizeof(info))) {
> -			ret = -EFAULT;
> -			goto out;
> -		}
> +	/* pre-format output fields to sane values */
> +	for (i = 0; i < same->dest_count; i++) {
> +		same->info[i].bytes_deduped = 0ULL;
> +		same->info[i].status = 0;
> +	}
>  
> -		info.bytes_deduped = 0;
> +	ret = 0;
> +	for (i = 0; i < same->dest_count; i++) {
> +		info = &same->info[i];
>  
> -		dst_file = fget(info.fd);
> +		dst_file = fget(info->fd);
>  		if (!dst_file) {
> -			info.status = -EBADF;
> +			info->status = -EBADF;
>  			goto next;
>  		}
>  
>  		if (!(is_admin || (dst_file->f_mode & FMODE_WRITE))) {
> -			info.status = -EINVAL;
> +			info->status = -EINVAL;
>  			goto next;
>  		}
>  
> -		info.status = -EXDEV;
> +		info->status = -EXDEV;
>  		if (file->f_path.mnt != dst_file->f_path.mnt)
>  			goto next;
>  
> @@ -2781,32 +2798,29 @@ static long btrfs_ioctl_file_extent_same(struct file *file,
>  			goto next;
>  
>  		if (S_ISDIR(dst->i_mode)) {
> -			info.status = -EISDIR;
> +			info->status = -EISDIR;
>  			goto next;
>  		}
>  
>  		if (!S_ISREG(dst->i_mode)) {
> -			info.status = -EACCES;
> +			info->status = -EACCES;
>  			goto next;
>  		}
>  
> -		info.status = btrfs_extent_same(src, off, len, dst,
> -						info.logical_offset);
> -		if (info.status == 0)
> -			info.bytes_deduped += len;
> +		info->status = btrfs_extent_same(src, off, len, dst,
> +						info->logical_offset);
> +		if (info->status == 0)
> +			info->bytes_deduped += len;
>  
>  next:
>  		if (dst_file)
>  			fput(dst_file);
> -
> -		if (__put_user_unaligned(info.status, &args->info[i].status) ||
> -		    __put_user_unaligned(info.bytes_deduped,
> -					 &args->info[i].bytes_deduped)) {
> -			ret = -EFAULT;
> -			goto out;
> -		}                                                               
>  	}
>  
> +	ret = copy_to_user(argp, same, size);
> +	if (ret)
> +		ret = -EFAULT;
> +
>  out:
>  	mnt_drop_write_file(file);
>  	return ret;
> -- 
> 1.8.1.4
> 
> 

Re: Build failures due to commit 416161db (btrfs: offline dedupe)

[Mark Fasheh]

On Wed, Sep 18, 2013 at 11:40:07AM -0700, Guenter Roeck wrote:
> On Tue, Sep 17, 2013 at 03:43:54PM -0700, Mark Fasheh wrote:
> > On Fri, Sep 13, 2013 at 03:33:34PM -0400, Chris Mason wrote:
> > > Mark, could you please send a patch for the whole-struct option until
> > > the unaligned put is upstreamed?
> > > 
> > > -chris
> > 
> > Here you go. It's been lightly tested and needs review.
> > 
> At the very least it does fix the build error on the affected platforms.

Thanks for verifying that Guenter.
	--Mark

--
Mark Fasheh