[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Andreas Gruenbacher]

There's no point in sharing the internal structure of lock value blocks
with user space.

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 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->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 (0x0000000000000000UL)
 #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	0x00000008
 #define GFS2_RGF_TRIMMED	0x00000010
 
-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;
 
-- 
2.20.1

[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Steven Whitehouse]

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 <agruenba@redhat.com>
> ---
>   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->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 (0x0000000000000000UL)
>   #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	0x00000008
>   #define GFS2_RGF_TRIMMED	0x00000010
>   
> -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;
>   

[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Andreas Gruenbacher]

On Wed, Jan 15, 2020 at 9:58 AM Steven Whitehouse <swhiteho@redhat.com> 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

[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Steven Whitehouse]

Hi,

On 15/01/2020 09:24, Andreas Gruenbacher wrote:
> On Wed, Jan 15, 2020 at 9:58 AM Steven Whitehouse <swhiteho@redhat.com> 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.

[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Bob Peterson]

----- Original Message -----
> Hi,
> 
> On 15/01/2020 09:24, Andreas Gruenbacher wrote:
> > On Wed, Jan 15, 2020 at 9:58 AM Steven Whitehouse <swhiteho@redhat.com>
> > 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 <those rpms> to all the nodes in the cluster
rpm --force -i --nodeps <those rpms>
/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

[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Andrew Price]

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 <swhiteho@redhat.com>
>>> 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 <those rpms> to all the nodes in the cluster
> rpm --force -i --nodeps <those rpms>
> /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

[Cluster-devel] [PATCH] Move struct gfs2_rgrp_lvb out of gfs2_ondisk.h

[Andrew Price]

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 <agruenba@redhat.com>
>> ---
>> ? 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->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 (0x0000000000000000UL)
>> ? #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??? 0x00000008
>> ? #define GFS2_RGF_TRIMMED??? 0x00000010
>> -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;
>