Guenter Roeck
2013-Sep-13 13:33 UTC
Build failures due to commit 416161db (btrfs: offline dedupe)
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
Geert Uytterhoeven
2013-Sep-13 13:52 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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
Guenter Roeck
2013-Sep-13 16:35 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Chris Mason
2013-Sep-13 17:00 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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 -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guenter Roeck
2013-Sep-13 17:15 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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
Mark Fasheh
2013-Sep-13 17:58 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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
Chris Mason
2013-Sep-13 19:33 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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 ;) > --MarkMark, could you please send a patch for the whole-struct option until the unaligned put is upstreamed? -chris
Mark Fasheh
2013-Sep-17 22:43 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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? > > -chrisHere 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
Guenter Roeck
2013-Sep-18 18:40 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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 > >
Mark Fasheh
2013-Sep-18 22:02 UTC
Re: Build failures due to commit 416161db (btrfs: offline dedupe)
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