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

Groups > linux.kernel > #1539877 > unrolled thread

[PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h James Simmons <jsimmons@infradead.org> - 2016-12-10 19:10 +0100
    Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-12-10 19:20 +0100
      Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h James Simmons <jsimmons@infradead.org> - 2016-12-12 21:10 +0100
        Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-12-13 01:40 +0100
          Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-13 09:40 +0100
            Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h Oleg Drokin <oleg.drokin@intel.com> - 2016-12-13 17:20 +0100
              Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h Dan Carpenter <dan.carpenter@oracle.com> - 2016-12-13 18:30 +0100
        Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h "Dilger, Andreas" <andreas.dilger@intel.com> - 2016-12-13 02:00 +0100
          Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-12-13 02:10 +0100
            Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder  functions in lustre_idl.h James Simmons <jsimmons@infradead.org> - 2016-12-19 18:10 +0100

#1539877 — [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

In order for lustre_idl.h to be usable for both user
land and kernel space it has to use the proper
byteorder functions.

Signed-off-by: James Simmons <uja.ornl@yahoo.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6245
Reviewed-on: http://review.whamcloud.com/16916
Reviewed-by: Frank Zago <fzago@cray.com>
Reviewed-by: Dmitry Eremin <dmitry.eremin@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
 .../lustre/lustre/include/lustre/lustre_idl.h      | 55 ++++++++++++----------
 1 file changed, 29 insertions(+), 26 deletions(-)

diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
index cd2dbfb..3d74d56 100644
--- a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
+++ b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
@@ -69,6 +69,9 @@
 #ifndef _LUSTRE_IDL_H_
 #define _LUSTRE_IDL_H_
 
+#include <asm/byteorder.h>
+#include <linux/types.h>
+
 #include "../../../include/linux/libcfs/libcfs.h"
 #include "../../../include/linux/lnet/types.h"
 
@@ -687,30 +690,30 @@ static inline void lu_igif_build(struct lu_fid *fid, __u32 ino, __u32 gen)
  */
 static inline void fid_cpu_to_le(struct lu_fid *dst, const struct lu_fid *src)
 {
-	dst->f_seq = cpu_to_le64(fid_seq(src));
-	dst->f_oid = cpu_to_le32(fid_oid(src));
-	dst->f_ver = cpu_to_le32(fid_ver(src));
+	dst->f_seq = __cpu_to_le64(fid_seq(src));
+	dst->f_oid = __cpu_to_le32(fid_oid(src));
+	dst->f_ver = __cpu_to_le32(fid_ver(src));
 }
 
 static inline void fid_le_to_cpu(struct lu_fid *dst, const struct lu_fid *src)
 {
-	dst->f_seq = le64_to_cpu(fid_seq(src));
-	dst->f_oid = le32_to_cpu(fid_oid(src));
-	dst->f_ver = le32_to_cpu(fid_ver(src));
+	dst->f_seq = __le64_to_cpu(fid_seq(src));
+	dst->f_oid = __le32_to_cpu(fid_oid(src));
+	dst->f_ver = __le32_to_cpu(fid_ver(src));
 }
 
 static inline void fid_cpu_to_be(struct lu_fid *dst, const struct lu_fid *src)
 {
-	dst->f_seq = cpu_to_be64(fid_seq(src));
-	dst->f_oid = cpu_to_be32(fid_oid(src));
-	dst->f_ver = cpu_to_be32(fid_ver(src));
+	dst->f_seq = __cpu_to_be64(fid_seq(src));
+	dst->f_oid = __cpu_to_be32(fid_oid(src));
+	dst->f_ver = __cpu_to_be32(fid_ver(src));
 }
 
 static inline void fid_be_to_cpu(struct lu_fid *dst, const struct lu_fid *src)
 {
-	dst->f_seq = be64_to_cpu(fid_seq(src));
-	dst->f_oid = be32_to_cpu(fid_oid(src));
-	dst->f_ver = be32_to_cpu(fid_ver(src));
+	dst->f_seq = __be64_to_cpu(fid_seq(src));
+	dst->f_oid = __be32_to_cpu(fid_oid(src));
+	dst->f_ver = __be32_to_cpu(fid_ver(src));
 }
 
 static inline bool fid_is_sane(const struct lu_fid *fid)
@@ -747,8 +750,8 @@ static inline void ostid_cpu_to_le(const struct ost_id *src_oi,
 				   struct ost_id *dst_oi)
 {
 	if (fid_seq_is_mdt0(ostid_seq(src_oi))) {
-		dst_oi->oi.oi_id = cpu_to_le64(src_oi->oi.oi_id);
-		dst_oi->oi.oi_seq = cpu_to_le64(src_oi->oi.oi_seq);
+		dst_oi->oi.oi_id = __cpu_to_le64(src_oi->oi.oi_id);
+		dst_oi->oi.oi_seq = __cpu_to_le64(src_oi->oi.oi_seq);
 	} else {
 		fid_cpu_to_le(&dst_oi->oi_fid, &src_oi->oi_fid);
 	}
@@ -758,8 +761,8 @@ static inline void ostid_le_to_cpu(const struct ost_id *src_oi,
 				   struct ost_id *dst_oi)
 {
 	if (fid_seq_is_mdt0(ostid_seq(src_oi))) {
-		dst_oi->oi.oi_id = le64_to_cpu(src_oi->oi.oi_id);
-		dst_oi->oi.oi_seq = le64_to_cpu(src_oi->oi.oi_seq);
+		dst_oi->oi.oi_id = __le64_to_cpu(src_oi->oi.oi_id);
+		dst_oi->oi.oi_seq = __le64_to_cpu(src_oi->oi.oi_seq);
 	} else {
 		fid_le_to_cpu(&dst_oi->oi_fid, &src_oi->oi_fid);
 	}
@@ -866,7 +869,7 @@ enum lu_dirpage_flags {
 
 static inline struct lu_dirent *lu_dirent_start(struct lu_dirpage *dp)
 {
-	if (le32_to_cpu(dp->ldp_flags) & LDF_EMPTY)
+	if (__le32_to_cpu(dp->ldp_flags) & LDF_EMPTY)
 		return NULL;
 	else
 		return dp->ldp_entries;
@@ -876,8 +879,8 @@ static inline struct lu_dirent *lu_dirent_next(struct lu_dirent *ent)
 {
 	struct lu_dirent *next;
 
-	if (le16_to_cpu(ent->lde_reclen) != 0)
-		next = ((void *)ent) + le16_to_cpu(ent->lde_reclen);
+	if (__le16_to_cpu(ent->lde_reclen) != 0)
+		next = ((void *)ent) + __le16_to_cpu(ent->lde_reclen);
 	else
 		next = NULL;
 
@@ -1438,15 +1441,15 @@ static inline __u64 lmm_oi_seq(const struct ost_id *oi)
 static inline void lmm_oi_le_to_cpu(struct ost_id *dst_oi,
 				    const struct ost_id *src_oi)
 {
-	dst_oi->oi.oi_id = le64_to_cpu(src_oi->oi.oi_id);
-	dst_oi->oi.oi_seq = le64_to_cpu(src_oi->oi.oi_seq);
+	dst_oi->oi.oi_id = __le64_to_cpu(src_oi->oi.oi_id);
+	dst_oi->oi.oi_seq = __le64_to_cpu(src_oi->oi.oi_seq);
 }
 
 static inline void lmm_oi_cpu_to_le(struct ost_id *dst_oi,
 				    const struct ost_id *src_oi)
 {
-	dst_oi->oi.oi_id = cpu_to_le64(src_oi->oi.oi_id);
-	dst_oi->oi.oi_seq = cpu_to_le64(src_oi->oi.oi_seq);
+	dst_oi->oi.oi_id = __cpu_to_le64(src_oi->oi.oi_id);
+	dst_oi->oi.oi_seq = __cpu_to_le64(src_oi->oi.oi_seq);
 }
 
 #define MAX_MD_SIZE							\
@@ -2382,11 +2385,11 @@ static inline ssize_t lmv_mds_md_size(int stripe_count, unsigned int lmm_magic)
 
 static inline int lmv_mds_md_stripe_count_get(const union lmv_mds_md *lmm)
 {
-	switch (le32_to_cpu(lmm->lmv_magic)) {
+	switch (__le32_to_cpu(lmm->lmv_magic)) {
 	case LMV_MAGIC_V1:
-		return le32_to_cpu(lmm->lmv_md_v1.lmv_stripe_count);
+		return __le32_to_cpu(lmm->lmv_md_v1.lmv_stripe_count);
 	case LMV_USER_MAGIC:
-		return le32_to_cpu(lmm->lmv_user_md.lum_stripe_count);
+		return __le32_to_cpu(lmm->lmv_user_md.lum_stripe_count);
 	default:
 		return -EINVAL;
 	}
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1539880 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> In order for lustre_idl.h to be usable for both user
> land and kernel space it has to use the proper
> byteorder functions.

Why would userspace need/want all of these inline functions?  A uapi
header file should just have a the structures that are passed
user/kernel and any needed ioctls.  Why would they ever care about
strange byte flip functions and a ton of inline functions?

I don't think this is needed, of if it is, I really don't want to see
your crazy userspace code...

thanks,

greg k-h

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

#1540584 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > In order for lustre_idl.h to be usable for both user
> > land and kernel space it has to use the proper
> > byteorder functions.
> 
> Why would userspace need/want all of these inline functions?  A uapi
> header file should just have a the structures that are passed
> user/kernel and any needed ioctls.  Why would they ever care about
> strange byte flip functions and a ton of inline functions?
> 
> I don't think this is needed, of if it is, I really don't want to see
> your crazy userspace code...

Sigh. More cleanups were done based on the idea this was okay. The reason
this was does was when you look at the headers in include/uapi/linux you
see a huge number of headers containing a bunch of inline function. To
an outside project looking to merge their work into the kernel they would
think this is okay. Hopefully all those broken headers will be cleaned
up in the near future. Alright I will look to fixing up our tools to 
handle this requirement. 

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

#1540764 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

On Mon, Dec 12, 2016 at 08:00:02PM +0000, James Simmons wrote:
> 
> > On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > > In order for lustre_idl.h to be usable for both user
> > > land and kernel space it has to use the proper
> > > byteorder functions.
> > 
> > Why would userspace need/want all of these inline functions?  A uapi
> > header file should just have a the structures that are passed
> > user/kernel and any needed ioctls.  Why would they ever care about
> > strange byte flip functions and a ton of inline functions?
> > 
> > I don't think this is needed, of if it is, I really don't want to see
> > your crazy userspace code...
> 
> Sigh. More cleanups were done based on the idea this was okay. The reason
> this was does was when you look at the headers in include/uapi/linux you
> see a huge number of headers containing a bunch of inline function. To
> an outside project looking to merge their work into the kernel they would
> think this is okay. Hopefully all those broken headers will be cleaned
> up in the near future. Alright I will look to fixing up our tools to 
> handle this requirement. 

But why do you need this type of stuff at all for your userspace code?
Why do they need these "complex" inline functions?  That implies that
there is duplicated logic on both sides of the user/kernel boundry,
shouldn't that be resolved somehow?

thanks,

greg k-h

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

#1540897 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

It used to be that great swathes of Lustre were used in both user space
and kernel space.  We had huge unused modules in the kernel that were
only used for user space.

regards,
dan carpenter

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

#1541220

On Dec 13, 2016, at 3:31 AM, Dan Carpenter wrote:

> It used to be that great swathes of Lustre were used in both user space
> and kernel space.  We had huge unused modules in the kernel that were
> only used for user space.

Huh?
There was nothing of the sort.
There were huge parts of code that were used by the server, but sue to no server
in staging client, ended up being unused, though.

There were also (much smaller) bits that were supporting userspace client
(that is, a library that was able to mount lustre servers completely from
userspace by hijacking libc calls), but that was mostly gone by the time
we got into staging anyway.

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

#1541303 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

On Tue, Dec 13, 2016 at 11:14:26AM -0500, Oleg Drokin wrote:
> 
> On Dec 13, 2016, at 3:31 AM, Dan Carpenter wrote:
> 
> > It used to be that great swathes of Lustre were used in both user space
> > and kernel space.  We had huge unused modules in the kernel that were
> > only used for user space.
> 
> Huh?
> There was nothing of the sort.
> There were huge parts of code that were used by the server, but sue to no server
> in staging client, ended up being unused, though.
> 

Oh.  Right,  that's it.

regards,
dan carpenter

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

#1540766 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

On Dec 12, 2016, at 13:00, James Simmons <jsimmons@infradead.org> wrote:
> 
> 
>> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
>>> In order for lustre_idl.h to be usable for both user
>>> land and kernel space it has to use the proper
>>> byteorder functions.
>> 
>> Why would userspace need/want all of these inline functions?  A uapi
>> header file should just have a the structures that are passed
>> user/kernel and any needed ioctls.  Why would they ever care about
>> strange byte flip functions and a ton of inline functions?
>> 
>> I don't think this is needed, of if it is, I really don't want to see
>> your crazy userspace code...
> 
> Sigh. More cleanups were done based on the idea this was okay. The
> reason this was does was when you look at the headers in
> include/uapi/linux you see a huge number of headers containing a bunch
> of inline function. To an outside project looking to merge their work
> into the kernel they would think this is okay. Hopefully all those
> broken headers will be cleaned up in the near future.
> Alright I will look to fixing up our tools to handle this requirement. 

These accessor functions are used by both the kernel and userspace
tools, and keeping them in the lustre_idl.h header avoids duplication
of code.  Similar usage exists in other filesystem related uapi headers
(e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).

That said, if there is an objection to keeping these macros/inline funcs
in the uapi headers, they still need to exist in the kernel and should
be kept in the lustre/include/lustre directory and we'll keep a separate
copy of the macros for userspace.

Cheers, Andreas

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

#1540780 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

On Tue, Dec 13, 2016 at 12:55:01AM +0000, Dilger, Andreas wrote:
> On Dec 12, 2016, at 13:00, James Simmons <jsimmons@infradead.org> wrote:
> > 
> > 
> >> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> >>> In order for lustre_idl.h to be usable for both user
> >>> land and kernel space it has to use the proper
> >>> byteorder functions.
> >> 
> >> Why would userspace need/want all of these inline functions?  A uapi
> >> header file should just have a the structures that are passed
> >> user/kernel and any needed ioctls.  Why would they ever care about
> >> strange byte flip functions and a ton of inline functions?
> >> 
> >> I don't think this is needed, of if it is, I really don't want to see
> >> your crazy userspace code...
> > 
> > Sigh. More cleanups were done based on the idea this was okay. The
> > reason this was does was when you look at the headers in
> > include/uapi/linux you see a huge number of headers containing a bunch
> > of inline function. To an outside project looking to merge their work
> > into the kernel they would think this is okay. Hopefully all those
> > broken headers will be cleaned up in the near future.
> > Alright I will look to fixing up our tools to handle this requirement. 
> 
> These accessor functions are used by both the kernel and userspace
> tools, and keeping them in the lustre_idl.h header avoids duplication
> of code.  Similar usage exists in other filesystem related uapi headers
> (e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).
> 
> That said, if there is an objection to keeping these macros/inline funcs
> in the uapi headers, they still need to exist in the kernel and should
> be kept in the lustre/include/lustre directory and we'll keep a separate
> copy of the macros for userspace.

"simple" accessors/setters are fine, but these start to get complex, you
are using unlikely, and debug macros and lots of other fun stuff.  Do
all other filesystems also do complex stuff like ostid_to_fid()?

thanks,

greg k-h

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

#1544673 — Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h

> On Tue, Dec 13, 2016 at 12:55:01AM +0000, Dilger, Andreas wrote:
> > On Dec 12, 2016, at 13:00, James Simmons <jsimmons@infradead.org> wrote:
> > > 
> > > 
> > >> On Sat, Dec 10, 2016 at 01:06:01PM -0500, James Simmons wrote:
> > >>> In order for lustre_idl.h to be usable for both user
> > >>> land and kernel space it has to use the proper
> > >>> byteorder functions.
> > >> 
> > >> Why would userspace need/want all of these inline functions?  A uapi
> > >> header file should just have a the structures that are passed
> > >> user/kernel and any needed ioctls.  Why would they ever care about
> > >> strange byte flip functions and a ton of inline functions?
> > >> 
> > >> I don't think this is needed, of if it is, I really don't want to see
> > >> your crazy userspace code...
> > > 
> > > Sigh. More cleanups were done based on the idea this was okay. The
> > > reason this was does was when you look at the headers in
> > > include/uapi/linux you see a huge number of headers containing a bunch
> > > of inline function. To an outside project looking to merge their work
> > > into the kernel they would think this is okay. Hopefully all those
> > > broken headers will be cleaned up in the near future.
> > > Alright I will look to fixing up our tools to handle this requirement. 
> > 
> > These accessor functions are used by both the kernel and userspace
> > tools, and keeping them in the lustre_idl.h header avoids duplication
> > of code.  Similar usage exists in other filesystem related uapi headers
> > (e.g. auto_fs4.h, bcache.h, btrfs_tree.h, nilfs2_ondisk.h, swab.h, etc.).
> > 
> > That said, if there is an objection to keeping these macros/inline funcs
> > in the uapi headers, they still need to exist in the kernel and should
> > be kept in the lustre/include/lustre directory and we'll keep a separate
> > copy of the macros for userspace.
> 
> "simple" accessors/setters are fine, but these start to get complex, you
> are using unlikely, and debug macros and lots of other fun stuff.  Do
> all other filesystems also do complex stuff like ostid_to_fid()?

So the rejection of the byteorder patch was more due to the state of 
headers than the patch itself. I do have other patches with the cleanup
of debugging macros etc but I was submitting the change one change at a
time. I will post what cleanups I was looking to do for  lustre_ostid.h 
and lustre_fid.h UAPI headers. This way you can give feedback on what is 
okay and what has to change.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web