Re: [Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h
On 15/01/2020 08:58, Steven Whitehouse wrote:
Hi,
On 15/01/2020 08:49, Andreas Gruenbacher wrote:
There's no point in sharing the internal structure of lock value blocks
with user space.
The reason that is in ondisk is that changing that structure is
something that needs to follow the same rules as changing the on disk
structures. So it is there as a reminder of that,
Perhaps some eye-catching code comments would suffice? Defining it in a
uapi header does sort-of communicate that it's ok to use in userspace,
whereas just having the structs in close proximity doesn't really
communicate that they should be updated in sync.
Andy
Signed-off-by: Andreas Gruenbacher
---
fs/gfs2/glock.h | 1 +
fs/gfs2/incore.h | 1 +
fs/gfs2/rgrp.c | 10 ++
include/uapi/linux/gfs2_ondisk.h | 10 --
4 files changed, 12 insertions(+), 10 deletions(-)
Steve.
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index b8adaf80e4c5..d2f2dba05a94 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -306,4 +306,5 @@ static inline void glock_clear_object(struct
gfs2_glock *gl, void *object)
spin_unlock(>gl_lockref.lock);
}
+
#endif /* __GLOCK_DOT_H__ */
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index b5d9c11f4901..5155389e9b5c 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -33,6 +33,7 @@ struct gfs2_trans;
struct gfs2_jdesc;
struct gfs2_sbd;
struct lm_lockops;
+struct gfs2_rgrp_lvb;
typedef void (*gfs2_glop_bh_t) (struct gfs2_glock *gl, unsigned int
ret);
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 2466bb44a23c..1165627274cf 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -46,6 +46,16 @@
#define LBITSKIP00 (0xUL)
#endif
+struct gfs2_rgrp_lvb {
+ __be32 rl_magic;
+ __be32 rl_flags;
+ __be32 rl_free;
+ __be32 rl_dinodes;
+ __be64 rl_igeneration;
+ __be32 rl_unlinked;
+ __be32 __pad;
+};
+
/*
* These routines are used by the resource group routines (rgrp.c)
* to keep track of block allocation. Each block is represented by two
diff --git a/include/uapi/linux/gfs2_ondisk.h
b/include/uapi/linux/gfs2_ondisk.h
index 2dc10a034de1..4e9a80941bec 100644
--- a/include/uapi/linux/gfs2_ondisk.h
+++ b/include/uapi/linux/gfs2_ondisk.h
@@ -171,16 +171,6 @@ struct gfs2_rindex {
#define GFS2_RGF_NOALLOC 0x0008
#define GFS2_RGF_TRIMMED 0x0010
-struct gfs2_rgrp_lvb {
- __be32 rl_magic;
- __be32 rl_flags;
- __be32 rl_free;
- __be32 rl_dinodes;
- __be64 rl_igeneration;
- __be32 rl_unlinked;
- __be32 __pad;
-};
-
struct gfs2_rgrp {
struct gfs2_meta_header rg_header;
Re: [Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h
On 15/01/2020 13:19, Bob Peterson wrote: - Original Message - Hi, On 15/01/2020 09:24, Andreas Gruenbacher wrote: On Wed, Jan 15, 2020 at 9:58 AM Steven Whitehouse wrote: On 15/01/2020 08:49, Andreas Gruenbacher wrote: There's no point in sharing the internal structure of lock value blocks with user space. The reason that is in ondisk is that changing that structure is something that needs to follow the same rules as changing the on disk structures. So it is there as a reminder of that, I can see a point in that. The reason I've posted this is because Bob was complaining that changes to include/uapi/linux/gfs2_ondisk.h break his out-of-tree module build process. (One of the patches I'm working on adds an inode LVB.) The same would be true of on-disk format changes as well of course, and those definitely need to be shared with user space. I'm not usually building gfs2 out of tree, so I'm indifferent to this change. Thanks, Andreas Why would we need to be able to build gfs2 (at least I assume it is gfs2) out of tree anyway? Steve. Simply for productivity. The difference is this procedure, which literally takes 10 seconds, if done simultaneously on all nodes using something like cssh: make -C /usr/src/kernels/4.18.0-165.el8.x86_64 modules M=$PWD I'd be concerned about this generating "chimera" modules that produce invalid test results. rmmod gfs2 insmod gfs2.ko Compared to a procedure like this, which takes at least 30 minutes: make (a new kernel .src.rpm) scp or rsync the .src.rpm to a build machine cd ~/rpmbuild/ rpm --force -i --nodeps /home/bob/*kernel-4.18.0*.src.rpm &> /dev/null echo $? rpmbuild --target=x86_64 -ba SPECS/kernel.spec ( -or- submit a "real" kernel build) then wait for the kernel build Pull down all necessary kernel rpms scp to all the nodes in the cluster rpm --force -i --nodeps /sbin/reboot all the nodes in the cluster wait for all the nodes to reboot, the cluster to stabilize, etc. Isn't the next-best alternative just building the modules in-tree and copying them to the test machines? I'm not sure I understand the complication. Perhaps we need cluster_install and cluster_modules_install rules in the build system :) Andy
Re: [Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h
- Original Message - > Hi, > > On 15/01/2020 09:24, Andreas Gruenbacher wrote: > > On Wed, Jan 15, 2020 at 9:58 AM Steven Whitehouse > > wrote: > >> On 15/01/2020 08:49, Andreas Gruenbacher wrote: > >>> There's no point in sharing the internal structure of lock value blocks > >>> with user space. > >> The reason that is in ondisk is that changing that structure is > >> something that needs to follow the same rules as changing the on disk > >> structures. So it is there as a reminder of that, > > I can see a point in that. The reason I've posted this is because Bob > > was complaining that changes to include/uapi/linux/gfs2_ondisk.h break > > his out-of-tree module build process. (One of the patches I'm working > > on adds an inode LVB.) The same would be true of on-disk format > > changes as well of course, and those definitely need to be shared with > > user space. I'm not usually building gfs2 out of tree, so I'm > > indifferent to this change. > > > > Thanks, > > Andreas > > > Why would we need to be able to build gfs2 (at least I assume it is > gfs2) out of tree anyway? > > Steve. Simply for productivity. The difference is this procedure, which literally takes 10 seconds, if done simultaneously on all nodes using something like cssh: make -C /usr/src/kernels/4.18.0-165.el8.x86_64 modules M=$PWD rmmod gfs2 insmod gfs2.ko Compared to a procedure like this, which takes at least 30 minutes: make (a new kernel .src.rpm) scp or rsync the .src.rpm to a build machine cd ~/rpmbuild/ rpm --force -i --nodeps /home/bob/*kernel-4.18.0*.src.rpm &> /dev/null echo $? rpmbuild --target=x86_64 -ba SPECS/kernel.spec ( -or- submit a "real" kernel build) then wait for the kernel build Pull down all necessary kernel rpms scp to all the nodes in the cluster rpm --force -i --nodeps /sbin/reboot all the nodes in the cluster wait for all the nodes to reboot, the cluster to stabilize, etc. Regards, Bob Peterson Red Hat File Systems
Re: [Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h
Hi, On 15/01/2020 09:24, Andreas Gruenbacher wrote: On Wed, Jan 15, 2020 at 9:58 AM Steven Whitehouse wrote: On 15/01/2020 08:49, Andreas Gruenbacher wrote: There's no point in sharing the internal structure of lock value blocks with user space. The reason that is in ondisk is that changing that structure is something that needs to follow the same rules as changing the on disk structures. So it is there as a reminder of that, I can see a point in that. The reason I've posted this is because Bob was complaining that changes to include/uapi/linux/gfs2_ondisk.h break his out-of-tree module build process. (One of the patches I'm working on adds an inode LVB.) The same would be true of on-disk format changes as well of course, and those definitely need to be shared with user space. I'm not usually building gfs2 out of tree, so I'm indifferent to this change. Thanks, Andreas Why would we need to be able to build gfs2 (at least I assume it is gfs2) out of tree anyway? Steve.
Re: [Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h
On Wed, Jan 15, 2020 at 9:58 AM Steven Whitehouse wrote: > On 15/01/2020 08:49, Andreas Gruenbacher wrote: > > There's no point in sharing the internal structure of lock value blocks > > with user space. > > The reason that is in ondisk is that changing that structure is > something that needs to follow the same rules as changing the on disk > structures. So it is there as a reminder of that, I can see a point in that. The reason I've posted this is because Bob was complaining that changes to include/uapi/linux/gfs2_ondisk.h break his out-of-tree module build process. (One of the patches I'm working on adds an inode LVB.) The same would be true of on-disk format changes as well of course, and those definitely need to be shared with user space. I'm not usually building gfs2 out of tree, so I'm indifferent to this change. Thanks, Andreas
Re: [Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h
Hi,
On 15/01/2020 08:49, Andreas Gruenbacher wrote:
There's no point in sharing the internal structure of lock value blocks
with user space.
The reason that is in ondisk is that changing that structure is
something that needs to follow the same rules as changing the on disk
structures. So it is there as a reminder of that,
Steve.
Signed-off-by: Andreas Gruenbacher
---
fs/gfs2/glock.h | 1 +
fs/gfs2/incore.h | 1 +
fs/gfs2/rgrp.c | 10 ++
include/uapi/linux/gfs2_ondisk.h | 10 --
4 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/fs/gfs2/glock.h b/fs/gfs2/glock.h
index b8adaf80e4c5..d2f2dba05a94 100644
--- a/fs/gfs2/glock.h
+++ b/fs/gfs2/glock.h
@@ -306,4 +306,5 @@ static inline void glock_clear_object(struct gfs2_glock
*gl, void *object)
spin_unlock(>gl_lockref.lock);
}
+
#endif /* __GLOCK_DOT_H__ */
diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index b5d9c11f4901..5155389e9b5c 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -33,6 +33,7 @@ struct gfs2_trans;
struct gfs2_jdesc;
struct gfs2_sbd;
struct lm_lockops;
+struct gfs2_rgrp_lvb;
typedef void (*gfs2_glop_bh_t) (struct gfs2_glock *gl, unsigned int ret);
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 2466bb44a23c..1165627274cf 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -46,6 +46,16 @@
#define LBITSKIP00 (0xUL)
#endif
+struct gfs2_rgrp_lvb {
+ __be32 rl_magic;
+ __be32 rl_flags;
+ __be32 rl_free;
+ __be32 rl_dinodes;
+ __be64 rl_igeneration;
+ __be32 rl_unlinked;
+ __be32 __pad;
+};
+
/*
* These routines are used by the resource group routines (rgrp.c)
* to keep track of block allocation. Each block is represented by two
diff --git a/include/uapi/linux/gfs2_ondisk.h b/include/uapi/linux/gfs2_ondisk.h
index 2dc10a034de1..4e9a80941bec 100644
--- a/include/uapi/linux/gfs2_ondisk.h
+++ b/include/uapi/linux/gfs2_ondisk.h
@@ -171,16 +171,6 @@ struct gfs2_rindex {
#define GFS2_RGF_NOALLOC 0x0008
#define GFS2_RGF_TRIMMED 0x0010
-struct gfs2_rgrp_lvb {
- __be32 rl_magic;
- __be32 rl_flags;
- __be32 rl_free;
- __be32 rl_dinodes;
- __be64 rl_igeneration;
- __be32 rl_unlinked;
- __be32 __pad;
-};
-
struct gfs2_rgrp {
struct gfs2_meta_header rg_header;
