[Cluster-devel] [PATCH] gfs2: Fix an incorrect gfs2_assert()

[Cluster-devel] [PATCH] gfs2: Fix an incorrect gfs2_assert()

[Tim Smith]

When updating the inode information after a change in allocation,
convert the change into the same units as the inode's i_blocks count
before comparing it in an assertion.

Also, change the comparison so that it is still possible to set i_blocks
to zero by adding -i_blocks, something that was previously only possible
because of the difference in units.

Signed-off-by: Tim Smith <tim.smith@citrix.com>
---
 fs/gfs2/inode.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/inode.h b/fs/gfs2/inode.h
index 793808263c6d..c8d0b05cf6ac 100644
--- a/fs/gfs2/inode.h
+++ b/fs/gfs2/inode.h
@@ -59,8 +59,8 @@ static inline u64 gfs2_get_inode_blocks(const struct inode *inode)
 
 static inline void gfs2_add_inode_blocks(struct inode *inode, s64 change)
 {
-	gfs2_assert(GFS2_SB(inode), (change >= 0 || inode->i_blocks > -change));
-	change *= (GFS2_SB(inode)->sd_sb.sb_bsize/GFS2_BASIC_BLOCK);
+	change <<= (GFS2_SB(inode)->sd_sb.sb_bsize_shift - GFS2_BASIC_BLOCK_SHIFT);
+	gfs2_assert(GFS2_SB(inode), (change >= 0 || inode->i_blocks >= -change));
 	inode->i_blocks += change;
 }
 
-- 
2.17.2


-- 
Tim Smith <tim.smith@citrix.com>

[Cluster-devel] [PATCH] gfs2: Fix an incorrect gfs2_assert()

[Bob Peterson]

----- Original Message -----
> When updating the inode information after a change in allocation,
> convert the change into the same units as the inode's i_blocks count
> before comparing it in an assertion.
> 
> Also, change the comparison so that it is still possible to set i_blocks
> to zero by adding -i_blocks, something that was previously only possible
> because of the difference in units.
> 
> Signed-off-by: Tim Smith <tim.smith@citrix.com>
> ---
>  fs/gfs2/inode.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/gfs2/inode.h b/fs/gfs2/inode.h
> index 793808263c6d..c8d0b05cf6ac 100644
> --- a/fs/gfs2/inode.h
> +++ b/fs/gfs2/inode.h
> @@ -59,8 +59,8 @@ static inline u64 gfs2_get_inode_blocks(const struct inode
> *inode)
>  
>  static inline void gfs2_add_inode_blocks(struct inode *inode, s64 change)
>  {
> -	gfs2_assert(GFS2_SB(inode), (change >= 0 || inode->i_blocks > -change));
> -	change *= (GFS2_SB(inode)->sd_sb.sb_bsize/GFS2_BASIC_BLOCK);
> +	change <<= (GFS2_SB(inode)->sd_sb.sb_bsize_shift - GFS2_BASIC_BLOCK_SHIFT);
> +	gfs2_assert(GFS2_SB(inode), (change >= 0 || inode->i_blocks >= -change));
>  	inode->i_blocks += change;
>  }
>  
> --
> 2.17.2
> 
> 
> --
> Tim Smith <tim.smith@citrix.com>

Hi Tim,

Did you see Andreas's patch posted on the 9th? It does the same exact thing,
but it's a little bit simpler and uses fewer levels of pointer indirection.

Yours:   change <<= (GFS2_SB(inode)->sd_sb.sb_bsize_shift - GFS2_BASIC_BLOCK_SHIFT);
Andreas: change <<= inode->i_blkbits - 9;

I suppose we could compromise and do:

         change <<= inode->i_blkbits - GFS2_BASIC_BLOCK_SHIFT;

Unless you object or have a better idea, I'll push Andrea's patch.

Regards,

Bob Peterson
Red Hat File Systems