* [PATCH v2 1/5] staging: lustre: obdclass: Create a header for obdo related functions
2016-12-10 18:05 [PATCH v2 0/5] Fix ups to make lustre_idl.h a proper UAPI header James Simmons
@ 2016-12-10 18:05 ` James Simmons
2016-12-10 18:05 ` [PATCH v2 2/5] staging: lustre: obdclass: style cleanup " James Simmons
` (3 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: James Simmons @ 2016-12-10 18:05 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Andreas Dilger, Oleg Drokin
Cc: Linux Kernel Mailing List, Lustre Development List, Ben Evans,
James Simmons
From: Ben Evans <bevans@cray.com>
Remove all obdo related functions from lustre_idl.h
Create lustre_odbo.h. Include where appropriate.
Make the functions lustre_get_wire_obdo and
lustre_set_wire_obdo to not be inlined functions.
Signed-off-by: Ben Evans <bevans@cray.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6401
Reviewed-on: http://review.whamcloud.com/16917
Reviewed-on: http://review.whamcloud.com/19266
Reviewed-by: James Simmons <uja.ornl@yahoo.com>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
Changelog:
v1) Initial patch from earlier version that broke build
v2) Include lustre_obdo.h to osc_request.c so it can
build. Next move style changes from moved functions
to separate patch.
.../lustre/lustre/include/lustre/lustre_idl.h | 46 ------------------
.../staging/lustre/lustre/include/lustre_obdo.h | 54 ++++++++++++++++++++++
drivers/staging/lustre/lustre/obdclass/obdo.c | 53 +++++++++++++++++++++
drivers/staging/lustre/lustre/osc/osc_io.c | 2 +
drivers/staging/lustre/lustre/osc/osc_request.c | 1 +
5 files changed, 110 insertions(+), 46 deletions(-)
create mode 100644 drivers/staging/lustre/lustre/include/lustre_obdo.h
diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
index 65ce503..b0eb80d 100644
--- a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
+++ b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
@@ -3130,52 +3130,6 @@ struct obdo {
#define o_cksum o_nlink
#define o_grant_used o_data_version
-static inline void lustre_set_wire_obdo(const struct obd_connect_data *ocd,
- struct obdo *wobdo,
- const struct obdo *lobdo)
-{
- *wobdo = *lobdo;
- wobdo->o_flags &= ~OBD_FL_LOCAL_MASK;
- if (!ocd)
- return;
-
- if (unlikely(!(ocd->ocd_connect_flags & OBD_CONNECT_FID)) &&
- fid_seq_is_echo(ostid_seq(&lobdo->o_oi))) {
- /* Currently OBD_FL_OSTID will only be used when 2.4 echo
- * client communicate with pre-2.4 server
- */
- wobdo->o_oi.oi.oi_id = fid_oid(&lobdo->o_oi.oi_fid);
- wobdo->o_oi.oi.oi_seq = fid_seq(&lobdo->o_oi.oi_fid);
- }
-}
-
-static inline void lustre_get_wire_obdo(const struct obd_connect_data *ocd,
- struct obdo *lobdo,
- const struct obdo *wobdo)
-{
- __u32 local_flags = 0;
-
- if (lobdo->o_valid & OBD_MD_FLFLAGS)
- local_flags = lobdo->o_flags & OBD_FL_LOCAL_MASK;
-
- *lobdo = *wobdo;
- if (local_flags != 0) {
- lobdo->o_valid |= OBD_MD_FLFLAGS;
- lobdo->o_flags &= ~OBD_FL_LOCAL_MASK;
- lobdo->o_flags |= local_flags;
- }
- if (!ocd)
- return;
-
- if (unlikely(!(ocd->ocd_connect_flags & OBD_CONNECT_FID)) &&
- fid_seq_is_echo(wobdo->o_oi.oi.oi_seq)) {
- /* see above */
- lobdo->o_oi.oi_fid.f_seq = wobdo->o_oi.oi.oi_seq;
- lobdo->o_oi.oi_fid.f_oid = wobdo->o_oi.oi.oi_id;
- lobdo->o_oi.oi_fid.f_ver = 0;
- }
-}
-
/* request structure for OST's */
struct ost_body {
struct obdo oa;
diff --git a/drivers/staging/lustre/lustre/include/lustre_obdo.h b/drivers/staging/lustre/lustre/include/lustre_obdo.h
new file mode 100644
index 0000000..1e12f8c
--- /dev/null
+++ b/drivers/staging/lustre/lustre/include/lustre_obdo.h
@@ -0,0 +1,54 @@
+/*
+ * GPL HEADER START
+ *
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 only,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License version 2 for more details (a copy is included
+ * in the LICENSE file that accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License
+ * version 2 along with this program; If not, see
+ * http://www.gnu.org/licenses/gpl-2.0.html
+ *
+ * GPL HEADER END
+ */
+/*
+ * Copyright (c) 2007, 2010, Oracle and/or its affiliates. All rights reserved.
+ * Use is subject to license terms.
+ *
+ * Copyright (c) 2011, 2014, Intel Corporation.
+ *
+ * Copyright 2015 Cray Inc, all rights reserved.
+ * Author: Ben Evans.
+ *
+ * Define obdo associated functions
+ * obdo: OBject Device o...
+ */
+
+#ifndef _LUSTRE_OBDO_H_
+#define _LUSTRE_OBDO_H_
+
+#include "lustre/lustre_idl.h"
+
+/**
+ * Create an obdo to send over the wire
+ */
+void lustre_set_wire_obdo(const struct obd_connect_data *ocd,
+ struct obdo *wobdo,
+ const struct obdo *lobdo);
+
+/**
+ * Create a local obdo from a wire based odbo
+ */
+void lustre_get_wire_obdo(const struct obd_connect_data *ocd,
+ struct obdo *lobdo,
+ const struct obdo *wobdo);
+
+#endif
diff --git a/drivers/staging/lustre/lustre/obdclass/obdo.c b/drivers/staging/lustre/lustre/obdclass/obdo.c
index c52b9e0..241e60b 100644
--- a/drivers/staging/lustre/lustre/obdclass/obdo.c
+++ b/drivers/staging/lustre/lustre/obdclass/obdo.c
@@ -40,6 +40,7 @@
#include "../include/obd_class.h"
#include "../include/lustre/lustre_idl.h"
+#include "../include/lustre_obdo.h"
void obdo_set_parent_fid(struct obdo *dst, const struct lu_fid *parent)
{
@@ -124,3 +125,55 @@ void obdo_to_ioobj(const struct obdo *oa, struct obd_ioobj *ioobj)
ioobj->ioo_max_brw = 0;
}
EXPORT_SYMBOL(obdo_to_ioobj);
+
+/**
+ * Create an obdo to send over the wire
+ */
+void lustre_set_wire_obdo(const struct obd_connect_data *ocd,
+ struct obdo *wobdo, const struct obdo *lobdo)
+{
+ *wobdo = *lobdo;
+ wobdo->o_flags &= ~OBD_FL_LOCAL_MASK;
+ if (!ocd)
+ return;
+
+ if (unlikely(!(ocd->ocd_connect_flags & OBD_CONNECT_FID)) &&
+ fid_seq_is_echo(ostid_seq(&lobdo->o_oi))) {
+ /* Currently OBD_FL_OSTID will only be used when 2.4 echo
+ * client communicate with pre-2.4 server
+ */
+ wobdo->o_oi.oi.oi_id = fid_oid(&lobdo->o_oi.oi_fid);
+ wobdo->o_oi.oi.oi_seq = fid_seq(&lobdo->o_oi.oi_fid);
+ }
+}
+EXPORT_SYMBOL(lustre_set_wire_obdo);
+
+/**
+ * Create a local obdo from a wire based odbo
+ */
+void lustre_get_wire_obdo(const struct obd_connect_data *ocd,
+ struct obdo *lobdo, const struct obdo *wobdo)
+{
+ __u32 local_flags = 0;
+
+ if (lobdo->o_valid & OBD_MD_FLFLAGS)
+ local_flags = lobdo->o_flags & OBD_FL_LOCAL_MASK;
+
+ *lobdo = *wobdo;
+ if (local_flags != 0) {
+ lobdo->o_valid |= OBD_MD_FLFLAGS;
+ lobdo->o_flags &= ~OBD_FL_LOCAL_MASK;
+ lobdo->o_flags |= local_flags;
+ }
+ if (!ocd)
+ return;
+
+ if (unlikely(!(ocd->ocd_connect_flags & OBD_CONNECT_FID)) &&
+ fid_seq_is_echo(wobdo->o_oi.oi.oi_seq)) {
+ /* see above */
+ lobdo->o_oi.oi_fid.f_seq = wobdo->o_oi.oi.oi_seq;
+ lobdo->o_oi.oi_fid.f_oid = wobdo->o_oi.oi.oi_id;
+ lobdo->o_oi.oi_fid.f_ver = 0;
+ }
+}
+EXPORT_SYMBOL(lustre_get_wire_obdo);
diff --git a/drivers/staging/lustre/lustre/osc/osc_io.c b/drivers/staging/lustre/lustre/osc/osc_io.c
index 228a97c..164aec3 100644
--- a/drivers/staging/lustre/lustre/osc/osc_io.c
+++ b/drivers/staging/lustre/lustre/osc/osc_io.c
@@ -37,6 +37,8 @@
#define DEBUG_SUBSYSTEM S_OSC
+#include "../include/lustre_obdo.h"
+
#include "osc_cl_internal.h"
/** \addtogroup osc
diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index 7143564..99aefa5 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -43,6 +43,7 @@
#include "../include/lprocfs_status.h"
#include "../include/lustre/lustre_ioctl.h"
#include "../include/lustre_debug.h"
+#include "../include/lustre_obdo.h"
#include "../include/lustre_param.h"
#include "../include/lustre_fid.h"
#include "../include/obd_class.h"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 2/5] staging: lustre: obdclass: style cleanup for obdo related functions
2016-12-10 18:05 [PATCH v2 0/5] Fix ups to make lustre_idl.h a proper UAPI header James Simmons
2016-12-10 18:05 ` [PATCH v2 1/5] staging: lustre: obdclass: Create a header for obdo related functions James Simmons
@ 2016-12-10 18:05 ` James Simmons
2016-12-10 18:05 ` [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move James Simmons
` (2 subsequent siblings)
4 siblings, 0 replies; 22+ messages in thread
From: James Simmons @ 2016-12-10 18:05 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Andreas Dilger, Oleg Drokin
Cc: Linux Kernel Mailing List, Lustre Development List, Ben Evans,
James Simmons
From: Ben Evans <bevans@cray.com>
Change the style of lustre_get_wire_obdo and
lustre_set_wire_obdo to conform to linux kernel
standard.
Signed-off-by: Ben Evans <bevans@cray.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6401
Reviewed-on: http://review.whamcloud.com/16917
Reviewed-on: http://review.whamcloud.com/19266
Reviewed-by: James Simmons <uja.ornl@yahoo.com>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
drivers/staging/lustre/lustre/obdclass/obdo.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/lustre/lustre/obdclass/obdo.c b/drivers/staging/lustre/lustre/obdclass/obdo.c
index 241e60b..b1dfa16 100644
--- a/drivers/staging/lustre/lustre/obdclass/obdo.c
+++ b/drivers/staging/lustre/lustre/obdclass/obdo.c
@@ -139,7 +139,8 @@ void lustre_set_wire_obdo(const struct obd_connect_data *ocd,
if (unlikely(!(ocd->ocd_connect_flags & OBD_CONNECT_FID)) &&
fid_seq_is_echo(ostid_seq(&lobdo->o_oi))) {
- /* Currently OBD_FL_OSTID will only be used when 2.4 echo
+ /*
+ * Currently OBD_FL_OSTID will only be used when 2.4 echo
* client communicate with pre-2.4 server
*/
wobdo->o_oi.oi.oi_id = fid_oid(&lobdo->o_oi.oi_fid);
@@ -154,13 +155,13 @@ void lustre_set_wire_obdo(const struct obd_connect_data *ocd,
void lustre_get_wire_obdo(const struct obd_connect_data *ocd,
struct obdo *lobdo, const struct obdo *wobdo)
{
- __u32 local_flags = 0;
+ u32 local_flags = 0;
if (lobdo->o_valid & OBD_MD_FLFLAGS)
local_flags = lobdo->o_flags & OBD_FL_LOCAL_MASK;
*lobdo = *wobdo;
- if (local_flags != 0) {
+ if (local_flags) {
lobdo->o_valid |= OBD_MD_FLFLAGS;
lobdo->o_flags &= ~OBD_FL_LOCAL_MASK;
lobdo->o_flags |= local_flags;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-10 18:05 [PATCH v2 0/5] Fix ups to make lustre_idl.h a proper UAPI header James Simmons
2016-12-10 18:05 ` [PATCH v2 1/5] staging: lustre: obdclass: Create a header for obdo related functions James Simmons
2016-12-10 18:05 ` [PATCH v2 2/5] staging: lustre: obdclass: style cleanup " James Simmons
@ 2016-12-10 18:05 ` James Simmons
2016-12-10 18:14 ` Greg Kroah-Hartman
2016-12-10 18:06 ` [PATCH v2 4/5] staging: lustre: headers: Move functions out of lustre_idl.h James Simmons
2016-12-10 18:06 ` [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h James Simmons
4 siblings, 1 reply; 22+ messages in thread
From: James Simmons @ 2016-12-10 18:05 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Andreas Dilger, Oleg Drokin
Cc: Linux Kernel Mailing List, Lustre Development List, Ben Evans,
James Simmons
From: Ben Evans <bevans@cray.com>
It was found if you sort the headers alphabetically
that it reduced patch conflicts. This patch sorts
the headers alphabetically and also place linux
header first, then uapi header and finally the
lustre kernel headers.
Signed-off-by: Ben Evans <bevans@cray.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6401
Reviewed-on: http://review.whamcloud.com/16917
Reviewed-on: http://review.whamcloud.com/19266
Reviewed-by: James Simmons <uja.ornl@yahoo.com>
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Oleg Drokin <oleg.drokin@intel.com>
Signed-off-by: James Simmons <jsimmons@infradead.org>
---
Changelog:
v1) Initial patch
v2) rebase patch against newer base patch that now
includes lustre_obdo.h in osc_request.c.
drivers/staging/lustre/lustre/osc/osc_request.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/lustre/lustre/osc/osc_request.c b/drivers/staging/lustre/lustre/osc/osc_request.c
index 99aefa5..0273ccd 100644
--- a/drivers/staging/lustre/lustre/osc/osc_request.c
+++ b/drivers/staging/lustre/lustre/osc/osc_request.c
@@ -34,20 +34,21 @@
#include "../../include/linux/libcfs/libcfs.h"
-#include "../include/lustre_dlm.h"
-#include "../include/lustre_net.h"
+#include "../include/lustre/lustre_ioctl.h"
#include "../include/lustre/lustre_user.h"
-#include "../include/obd_cksum.h"
-#include "../include/lustre_ha.h"
#include "../include/lprocfs_status.h"
-#include "../include/lustre/lustre_ioctl.h"
#include "../include/lustre_debug.h"
+#include "../include/lustre_dlm.h"
+#include "../include/lustre_fid.h"
+#include "../include/lustre_ha.h"
+#include "../include/lustre_net.h"
#include "../include/lustre_obdo.h"
#include "../include/lustre_param.h"
-#include "../include/lustre_fid.h"
-#include "../include/obd_class.h"
#include "../include/obd.h"
+#include "../include/obd_cksum.h"
+#include "../include/obd_class.h"
+
#include "osc_internal.h"
#include "osc_cl_internal.h"
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-10 18:05 ` [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move James Simmons
@ 2016-12-10 18:14 ` Greg Kroah-Hartman
2016-12-12 14:42 ` Ben Evans
0 siblings, 1 reply; 22+ messages in thread
From: Greg Kroah-Hartman @ 2016-12-10 18:14 UTC (permalink / raw)
To: James Simmons
Cc: devel, Andreas Dilger, Oleg Drokin, Ben Evans,
Linux Kernel Mailing List, Lustre Development List
On Sat, Dec 10, 2016 at 01:05:59PM -0500, James Simmons wrote:
> From: Ben Evans <bevans@cray.com>
>
> It was found if you sort the headers alphabetically
> that it reduced patch conflicts. This patch sorts
> the headers alphabetically and also place linux
> header first, then uapi header and finally the
> lustre kernel headers.
I still don't agree, when did you last have a patch conflict with this
file in the .h section? And exactly how hard was it to fix it up?
I'm all for cleanups, but really, this is useless. And I said so the
last time you sent it...
greg k-h
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-10 18:14 ` Greg Kroah-Hartman
@ 2016-12-12 14:42 ` Ben Evans
2016-12-12 16:34 ` Greg Kroah-Hartman
0 siblings, 1 reply; 22+ messages in thread
From: Ben Evans @ 2016-12-12 14:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, James Simmons
Cc: devel, Andreas Dilger, Oleg Drokin, Ben Evans,
Linux Kernel Mailing List, Lustre Development List
This was done to conform to the Lustre Coding Guidelines.
-Ben
On 12/10/16, 1:14 PM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
wrote:
>On Sat, Dec 10, 2016 at 01:05:59PM -0500, James Simmons wrote:
>> From: Ben Evans <bevans@cray.com>
>>
>> It was found if you sort the headers alphabetically
>> that it reduced patch conflicts. This patch sorts
>> the headers alphabetically and also place linux
>> header first, then uapi header and finally the
>> lustre kernel headers.
>
>I still don't agree, when did you last have a patch conflict with this
>file in the .h section? And exactly how hard was it to fix it up?
>
>I'm all for cleanups, but really, this is useless. And I said so the
>last time you sent it...
>
>greg k-h
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-12 14:42 ` Ben Evans
@ 2016-12-12 16:34 ` Greg Kroah-Hartman
2016-12-12 18:12 ` Ben Evans
2016-12-12 18:19 ` Joe Perches
0 siblings, 2 replies; 22+ messages in thread
From: Greg Kroah-Hartman @ 2016-12-12 16:34 UTC (permalink / raw)
To: Ben Evans
Cc: James Simmons, devel, Andreas Dilger, Oleg Drokin,
Linux Kernel Mailing List, Lustre Development List
A: Top-posting.
Q: What is the most annoying thing in e-mail?
A: No.
Q: Should I include quotations after my reply?
http://daringfireball.net/2007/07/on_top
On Mon, Dec 12, 2016 at 02:42:29PM +0000, Ben Evans wrote:
> This was done to conform to the Lustre Coding Guidelines.
What is this mythical guidelines, and why does it differ from the kernel
source ones?
And again, why is this patch required?
thanks,
greg k-h
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-12 16:34 ` Greg Kroah-Hartman
@ 2016-12-12 18:12 ` Ben Evans
2016-12-12 18:19 ` Joe Perches
1 sibling, 0 replies; 22+ messages in thread
From: Ben Evans @ 2016-12-12 18:12 UTC (permalink / raw)
To: Greg Kroah-Hartman, Ben Evans
Cc: James Simmons, devel, Andreas Dilger, Oleg Drokin,
Linux Kernel Mailing List, Lustre Development List
On 12/12/16, 11:34 AM, "Greg Kroah-Hartman" <gregkh@linuxfoundation.org>
wrote:
>What is this mythical guidelines, and why does it differ from the kernel
>source ones?
>
>And again, why is this patch required?
>
>thanks,
>
>greg k-h
>
Here are the general guidelines for your reading pleasure:
http://wiki.lustre.org/Lustre_Coding_Style_Guidelines
The specific guidelines on organizing #includes are here:
http://wiki.lustre.org/Lustre_Style_Guide_Includes
-Ben Evans
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-12 16:34 ` Greg Kroah-Hartman
2016-12-12 18:12 ` Ben Evans
@ 2016-12-12 18:19 ` Joe Perches
2016-12-12 19:41 ` James Simmons
1 sibling, 1 reply; 22+ messages in thread
From: Joe Perches @ 2016-12-12 18:19 UTC (permalink / raw)
To: Greg Kroah-Hartman, Ben Evans
Cc: James Simmons, devel, Andreas Dilger, Oleg Drokin,
Linux Kernel Mailing List, Lustre Development List
On Mon, 2016-12-12 at 17:34 +0100, Greg Kroah-Hartman wrote:
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
>
> On Mon, Dec 12, 2016 at 02:42:29PM +0000, Ben Evans wrote:
> > This was done to conform to the Lustre Coding Guidelines.
>
> What is this mythical guidelines, and why does it differ from the kernel
> source ones?
It's not like it's hard to find
http://wiki.lustre.org/Lustre_Coding_Style_Guidelines
And in specific:
http://wiki.lustre.org/Lustre_Style_Guide_Includes
There is no single mandated code style for this.
Some people like reverse christmas tree.
Whatever...
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-12 18:19 ` Joe Perches
@ 2016-12-12 19:41 ` James Simmons
2016-12-12 19:52 ` Joe Perches
0 siblings, 1 reply; 22+ messages in thread
From: James Simmons @ 2016-12-12 19:41 UTC (permalink / raw)
To: Joe Perches
Cc: Greg Kroah-Hartman, Ben Evans, devel, Andreas Dilger,
Oleg Drokin, Linux Kernel Mailing List, Lustre Development List
> On Mon, 2016-12-12 at 17:34 +0100, Greg Kroah-Hartman wrote:
> > A: Top-posting.
> > Q: What is the most annoying thing in e-mail?
> >
> > A: No.
> > Q: Should I include quotations after my reply?
> >
> > http://daringfireball.net/2007/07/on_top
> >
> > On Mon, Dec 12, 2016 at 02:42:29PM +0000, Ben Evans wrote:
> > > This was done to conform to the Lustre Coding Guidelines.
> >
> > What is this mythical guidelines, and why does it differ from the kernel
> > source ones?
>
> It's not like it's hard to find
> http://wiki.lustre.org/Lustre_Coding_Style_Guidelines
>
> And in specific:
> http://wiki.lustre.org/Lustre_Style_Guide_Includes
>
> There is no single mandated code style for this.
> Some people like reverse christmas tree.
>
> Whatever...
>
Sigh, Sayre's law. I went looking to see what the offical
policy is for this and found nothing. If this is really
important can we then place an offical policy on how
headers are added to a C file in CodingStyle and add
something to checkpatch to detect incorrect patches.
Lets burn down this bike shed once and for all.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move
2016-12-12 19:41 ` James Simmons
@ 2016-12-12 19:52 ` Joe Perches
0 siblings, 0 replies; 22+ messages in thread
From: Joe Perches @ 2016-12-12 19:52 UTC (permalink / raw)
To: James Simmons
Cc: Greg Kroah-Hartman, Ben Evans, devel, Andreas Dilger,
Oleg Drokin, Linux Kernel Mailing List, Lustre Development List
On Mon, 2016-12-12 at 19:41 +0000, James Simmons wrote:
> > On Mon, 2016-12-12 at 17:34 +0100, Greg Kroah-Hartman wrote:
[]
> > http://wiki.lustre.org/Lustre_Style_Guide_Includes
> >
> > There is no single mandated code style for this.
> > Some people like reverse christmas tree.
> >
> > Whatever...
> >
> Sigh, Sayre's law.
<chuckle>
> I went looking to see what the offical
> policy is for this and found nothing. If this is really
> important can we then place an offical policy on how
> headers are added to a C file in CodingStyle and add
> something to checkpatch to detect incorrect patches.
> Lets burn down this bike shed once and for all.
g'luck with that.
David Miller might like to have a word with you too.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH v2 4/5] staging: lustre: headers: Move functions out of lustre_idl.h
2016-12-10 18:05 [PATCH v2 0/5] Fix ups to make lustre_idl.h a proper UAPI header James Simmons
` (2 preceding siblings ...)
2016-12-10 18:05 ` [PATCH v2 3/5] staging: lustre: headers: sort headers affected by obdo move James Simmons
@ 2016-12-10 18:06 ` James Simmons
2016-12-10 18:06 ` [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h James Simmons
4 siblings, 0 replies; 22+ messages in thread
From: James Simmons @ 2016-12-10 18:06 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Andreas Dilger, Oleg Drokin
Cc: Linux Kernel Mailing List, Lustre Development List, Ben Evans,
James Simmons
From: Ben Evans <bevans@cray.com>
Migrate functions set/get_mrc_cr_flags, ldlm_res_eq
ldlm_extent_overlap, ldlm_extent_contain,
ldlm_request_bufsize, and alll the PTLRPC dump_*
functions out of lustre_idl.h which is a UAPI header
to the places in the kernel code they are actually used.
Delete unused lmv_mds_md_stripe_count and
agent_req_in_final_state.
Signed-off-by: Ben Evans <bevans@cray.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-6401
Reviewed-on: http://review.whamcloud.com/21484
Reviewed-by: Frank Zago <fzago@cray.com>
Reviewed-by: James Simmons <uja.ornl@yahoo.com>
Reviewed-by: Andreas Dilger <andreas.dilger@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 | 73 ----------------------
drivers/staging/lustre/lustre/include/lustre_dlm.h | 13 ++++
.../staging/lustre/lustre/include/lustre_swab.h | 6 ++
drivers/staging/lustre/lustre/ldlm/ldlm_internal.h | 6 ++
drivers/staging/lustre/lustre/ldlm/ldlm_request.c | 27 ++++++++
drivers/staging/lustre/lustre/mdc/mdc_lib.c | 6 ++
6 files changed, 58 insertions(+), 73 deletions(-)
diff --git a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
index b0eb80d..cd2dbfb 100644
--- a/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
+++ b/drivers/staging/lustre/lustre/include/lustre/lustre_idl.h
@@ -2130,17 +2130,6 @@ struct mdt_rec_create {
__u32 cr_padding_4; /* rr_padding_4 */
};
-static inline void set_mrc_cr_flags(struct mdt_rec_create *mrc, __u64 flags)
-{
- mrc->cr_flags_l = (__u32)(flags & 0xFFFFFFFFUll);
- mrc->cr_flags_h = (__u32)(flags >> 32);
-}
-
-static inline __u64 get_mrc_cr_flags(struct mdt_rec_create *mrc)
-{
- return ((__u64)(mrc->cr_flags_l) | ((__u64)mrc->cr_flags_h << 32));
-}
-
/* instance of mdt_reint_rec */
struct mdt_rec_link {
__u32 lk_opcode;
@@ -2403,25 +2392,6 @@ static inline int lmv_mds_md_stripe_count_get(const union lmv_mds_md *lmm)
}
}
-static inline int lmv_mds_md_stripe_count_set(union lmv_mds_md *lmm,
- unsigned int stripe_count)
-{
- int rc = 0;
-
- switch (le32_to_cpu(lmm->lmv_magic)) {
- case LMV_MAGIC_V1:
- lmm->lmv_md_v1.lmv_stripe_count = cpu_to_le32(stripe_count);
- break;
- case LMV_USER_MAGIC:
- lmm->lmv_user_md.lum_stripe_count = cpu_to_le32(stripe_count);
- break;
- default:
- rc = -EINVAL;
- break;
- }
- return rc;
-}
-
enum fld_rpc_opc {
FLD_QUERY = 900,
FLD_READ = 901,
@@ -2502,12 +2472,6 @@ struct ldlm_res_id {
#define PLDLMRES(res) (res)->lr_name.name[0], (res)->lr_name.name[1], \
(res)->lr_name.name[2], (res)->lr_name.name[3]
-static inline bool ldlm_res_eq(const struct ldlm_res_id *res0,
- const struct ldlm_res_id *res1)
-{
- return !memcmp(res0, res1, sizeof(*res0));
-}
-
/* lock types */
enum ldlm_mode {
LCK_MINMODE = 0,
@@ -2540,19 +2504,6 @@ struct ldlm_extent {
__u64 gid;
};
-static inline int ldlm_extent_overlap(const struct ldlm_extent *ex1,
- const struct ldlm_extent *ex2)
-{
- return (ex1->start <= ex2->end) && (ex2->start <= ex1->end);
-}
-
-/* check if @ex1 contains @ex2 */
-static inline int ldlm_extent_contain(const struct ldlm_extent *ex1,
- const struct ldlm_extent *ex2)
-{
- return (ex1->start <= ex2->start) && (ex1->end >= ex2->end);
-}
-
struct ldlm_inodebits {
__u64 bits;
};
@@ -2627,18 +2578,6 @@ struct ldlm_request {
struct lustre_handle lock_handle[LDLM_LOCKREQ_HANDLES];
};
-/* If LDLM_ENQUEUE, 1 slot is already occupied, 1 is available.
- * Otherwise, 2 are available.
- */
-#define ldlm_request_bufsize(count, type) \
-({ \
- int _avail = LDLM_LOCKREQ_HANDLES; \
- _avail -= (type == LDLM_ENQUEUE ? LDLM_ENQUEUE_CANCEL_OFF : 0); \
- sizeof(struct ldlm_request) + \
- (count > _avail ? count - _avail : 0) * \
- sizeof(struct lustre_handle); \
-})
-
struct ldlm_reply {
__u32 lock_flags;
__u32 lock_padding; /* also fix lustre_swab_ldlm_reply */
@@ -2942,12 +2881,6 @@ static inline const char *agent_req_status2name(const enum agent_req_status ars)
}
}
-static inline bool agent_req_in_final_state(enum agent_req_status ars)
-{
- return ((ars == ARS_SUCCEED) || (ars == ARS_FAILED) ||
- (ars == ARS_CANCELED));
-}
-
struct llog_agent_req_rec {
struct llog_rec_hdr arr_hdr; /**< record header */
__u32 arr_status; /**< status of the request */
@@ -3142,12 +3075,6 @@ struct ll_fiemap_info_key {
struct fiemap lfik_fiemap;
};
-/* Functions for dumping PTLRPC fields */
-void dump_rniobuf(struct niobuf_remote *rnb);
-void dump_ioo(struct obd_ioobj *nb);
-void dump_ost_body(struct ost_body *ob);
-void dump_rcs(__u32 *rc);
-
/* security opcodes */
enum sec_cmd {
SEC_CTX_INIT = 801,
diff --git a/drivers/staging/lustre/lustre/include/lustre_dlm.h b/drivers/staging/lustre/lustre/include/lustre_dlm.h
index b7e61d0..0d7eb80 100644
--- a/drivers/staging/lustre/lustre/include/lustre_dlm.h
+++ b/drivers/staging/lustre/lustre/include/lustre_dlm.h
@@ -1339,5 +1339,18 @@ int ldlm_pool_init(struct ldlm_pool *pl, struct ldlm_namespace *ns,
void ldlm_pool_del(struct ldlm_pool *pl, struct ldlm_lock *lock);
/** @} */
+static inline int ldlm_extent_overlap(const struct ldlm_extent *ex1,
+ const struct ldlm_extent *ex2)
+{
+ return ex1->start <= ex2->end && ex2->start <= ex1->end;
+}
+
+/* check if @ex1 contains @ex2 */
+static inline int ldlm_extent_contain(const struct ldlm_extent *ex1,
+ const struct ldlm_extent *ex2)
+{
+ return ex1->start <= ex2->start && ex1->end >= ex2->end;
+}
+
#endif
/** @} LDLM */
diff --git a/drivers/staging/lustre/lustre/include/lustre_swab.h b/drivers/staging/lustre/lustre/include/lustre_swab.h
index 26d01c2..5c1bdc0 100644
--- a/drivers/staging/lustre/lustre/include/lustre_swab.h
+++ b/drivers/staging/lustre/lustre/include/lustre_swab.h
@@ -99,4 +99,10 @@ void lustre_swab_lov_user_md_objects(struct lov_user_ost_data *lod,
void lustre_swab_close_data(struct close_data *data);
void lustre_swab_lmv_user_md(struct lmv_user_md *lum);
+/* Functions for dumping PTLRPC fields */
+void dump_rniobuf(struct niobuf_remote *rnb);
+void dump_ioo(struct obd_ioobj *nb);
+void dump_ost_body(struct ost_body *ob);
+void dump_rcs(__u32 *rc);
+
#endif
diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_internal.h b/drivers/staging/lustre/lustre/ldlm/ldlm_internal.h
index 5c02501..d3a9609 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_internal.h
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_internal.h
@@ -334,3 +334,9 @@ void ldlm_flock_policy_wire_to_local(const union ldlm_wire_policy_data *wpolicy,
union ldlm_policy_data *lpolicy);
void ldlm_flock_policy_local_to_wire(const union ldlm_policy_data *lpolicy,
union ldlm_wire_policy_data *wpolicy);
+
+static inline bool ldlm_res_eq(const struct ldlm_res_id *res0,
+ const struct ldlm_res_id *res1)
+{
+ return memcmp(res0, res1, sizeof(*res0)) == 0;
+}
diff --git a/drivers/staging/lustre/lustre/ldlm/ldlm_request.c b/drivers/staging/lustre/lustre/ldlm/ldlm_request.c
index c1f8693..cd6614b 100644
--- a/drivers/staging/lustre/lustre/ldlm/ldlm_request.c
+++ b/drivers/staging/lustre/lustre/ldlm/ldlm_request.c
@@ -83,6 +83,33 @@ struct ldlm_async_args {
struct lustre_handle lock_handle;
};
+/**
+ * ldlm_request_bufsize
+ *
+ * @count: number of ldlm handles
+ * @type: ldlm opcode
+ *
+ * If opcode=LDLM_ENQUEUE, 1 slot is already occupied,
+ * LDLM_LOCKREQ_HANDLE -1 slots are available.
+ * Otherwise, LDLM_LOCKREQ_HANDLE slots are available.
+ *
+ * Return: size of the request buffer
+ */
+int ldlm_request_bufsize(int count, int type)
+{
+ int avail = LDLM_LOCKREQ_HANDLES;
+
+ if (type == LDLM_ENQUEUE)
+ avail -= LDLM_ENQUEUE_CANCEL_OFF;
+
+ if (count > avail)
+ avail = (count - avail) * sizeof(struct lustre_handle);
+ else
+ avail = 0;
+
+ return sizeof(struct ldlm_request) + avail;
+}
+
static int ldlm_expired_completion_wait(void *data)
{
struct lock_wait_data *lwd = data;
diff --git a/drivers/staging/lustre/lustre/mdc/mdc_lib.c b/drivers/staging/lustre/lustre/mdc/mdc_lib.c
index f35e1f9..4e55255 100644
--- a/drivers/staging/lustre/lustre/mdc/mdc_lib.c
+++ b/drivers/staging/lustre/lustre/mdc/mdc_lib.c
@@ -35,6 +35,12 @@
#include "../include/lustre/lustre_idl.h"
#include "mdc_internal.h"
+static void set_mrc_cr_flags(struct mdt_rec_create *mrc, u64 flags)
+{
+ mrc->cr_flags_l = (u32)(flags & 0xFFFFFFFFUll);
+ mrc->cr_flags_h = (u32)(flags >> 32);
+}
+
static void __mdc_pack_body(struct mdt_body *b, __u32 suppgid)
{
b->mbo_suppgid = suppgid;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-10 18:05 [PATCH v2 0/5] Fix ups to make lustre_idl.h a proper UAPI header James Simmons
` (3 preceding siblings ...)
2016-12-10 18:06 ` [PATCH v2 4/5] staging: lustre: headers: Move functions out of lustre_idl.h James Simmons
@ 2016-12-10 18:06 ` James Simmons
2016-12-10 18:16 ` Greg Kroah-Hartman
4 siblings, 1 reply; 22+ messages in thread
From: James Simmons @ 2016-12-10 18:06 UTC (permalink / raw)
To: Greg Kroah-Hartman, devel, Andreas Dilger, Oleg Drokin
Cc: Linux Kernel Mailing List, Lustre Development List,
James Simmons, James Simmons
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
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-10 18:06 ` [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h James Simmons
@ 2016-12-10 18:16 ` Greg Kroah-Hartman
2016-12-12 20:00 ` James Simmons
0 siblings, 1 reply; 22+ messages in thread
From: Greg Kroah-Hartman @ 2016-12-10 18:16 UTC (permalink / raw)
To: James Simmons
Cc: devel, Andreas Dilger, Oleg Drokin, James Simmons,
Linux Kernel Mailing List, Lustre Development List
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-10 18:16 ` Greg Kroah-Hartman
@ 2016-12-12 20:00 ` James Simmons
2016-12-13 0:37 ` Greg Kroah-Hartman
2016-12-13 0:55 ` Dilger, Andreas
0 siblings, 2 replies; 22+ messages in thread
From: James Simmons @ 2016-12-12 20:00 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: devel, Andreas Dilger, Oleg Drokin, James Simmons,
Linux Kernel Mailing List, Lustre Development List
> 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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-12 20:00 ` James Simmons
@ 2016-12-13 0:37 ` Greg Kroah-Hartman
2016-12-13 8:31 ` Dan Carpenter
2016-12-13 0:55 ` Dilger, Andreas
1 sibling, 1 reply; 22+ messages in thread
From: Greg Kroah-Hartman @ 2016-12-13 0:37 UTC (permalink / raw)
To: James Simmons
Cc: devel, James Simmons, Andreas Dilger, Linux Kernel Mailing List,
Oleg Drokin, Lustre Development List
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-13 0:37 ` Greg Kroah-Hartman
@ 2016-12-13 8:31 ` Dan Carpenter
2016-12-13 16:14 ` Oleg Drokin
0 siblings, 1 reply; 22+ messages in thread
From: Dan Carpenter @ 2016-12-13 8:31 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: James Simmons, devel, James Simmons, Andreas Dilger,
Linux Kernel Mailing List, Oleg Drokin, Lustre Development List
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-13 8:31 ` Dan Carpenter
@ 2016-12-13 16:14 ` Oleg Drokin
2016-12-13 17:21 ` Dan Carpenter
0 siblings, 1 reply; 22+ messages in thread
From: Oleg Drokin @ 2016-12-13 16:14 UTC (permalink / raw)
To: Dan Carpenter
Cc: Greg Kroah-Hartman, James Simmons, devel, James Simmons,
Andreas Dilger, Linux Kernel Mailing List,
Lustre Development List
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.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-13 16:14 ` Oleg Drokin
@ 2016-12-13 17:21 ` Dan Carpenter
0 siblings, 0 replies; 22+ messages in thread
From: Dan Carpenter @ 2016-12-13 17:21 UTC (permalink / raw)
To: Oleg Drokin
Cc: devel, James Simmons, Greg Kroah-Hartman,
Linux Kernel Mailing List, Andreas Dilger,
Lustre Development List
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-12 20:00 ` James Simmons
2016-12-13 0:37 ` Greg Kroah-Hartman
@ 2016-12-13 0:55 ` Dilger, Andreas
2016-12-13 1:07 ` Greg Kroah-Hartman
1 sibling, 1 reply; 22+ messages in thread
From: Dilger, Andreas @ 2016-12-13 0:55 UTC (permalink / raw)
To: James Simmons
Cc: Greg Kroah-Hartman, devel, James Simmons,
Linux Kernel Mailing List, Drokin, Oleg, Lustre Development List
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-13 0:55 ` Dilger, Andreas
@ 2016-12-13 1:07 ` Greg Kroah-Hartman
2016-12-19 17:02 ` James Simmons
0 siblings, 1 reply; 22+ messages in thread
From: Greg Kroah-Hartman @ 2016-12-13 1:07 UTC (permalink / raw)
To: Dilger, Andreas
Cc: James Simmons, devel, James Simmons, Linux Kernel Mailing List,
Drokin, Oleg, Lustre Development List
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v2 5/5] staging: lustre: headers: use proper byteorder functions in lustre_idl.h
2016-12-13 1:07 ` Greg Kroah-Hartman
@ 2016-12-19 17:02 ` James Simmons
0 siblings, 0 replies; 22+ messages in thread
From: James Simmons @ 2016-12-19 17:02 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Dilger, Andreas, devel, James Simmons, Linux Kernel Mailing List,
Drokin, Oleg, Lustre Development List
> 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.
^ permalink raw reply [flat|nested] 22+ messages in thread