diff mbox series

[v2,1/8] ext4: use ASSERT() to replace J_ASSERT()

Message ID 1603098158-30406-1-git-send-email-brookxu@tencent.com
State Superseded
Headers show
Series [v2,1/8] ext4: use ASSERT() to replace J_ASSERT() | expand

Commit Message

Chunguang Xu Oct. 19, 2020, 9:02 a.m. UTC
There are currently multiple forms of assertion, such as J_ASSERT().
J_ASEERT is provided for the jbd module, which is a public module.
Maybe we should use custom ASSERT() like other file systems, such as
xfs, which would be better.

Signed-off-by: Chunguang Xu <brookxu@tencent.com>
---
 fs/ext4/balloc.c   |  2 +-
 fs/ext4/ext4.h     | 10 ++++++++++
 fs/ext4/fsync.c    |  2 +-
 fs/ext4/indirect.c |  4 ++--
 fs/ext4/inode.c    |  6 +++---
 fs/ext4/namei.c    | 12 ++++--------
 fs/ext4/super.c    |  2 +-
 7 files changed, 22 insertions(+), 16 deletions(-)

Comments

Andreas Dilger Oct. 20, 2020, 3:37 a.m. UTC | #1
On Oct 19, 2020, at 3:02 AM, Chunguang Xu <brookxu.cn@gmail.com> wrote:
> 
> There are currently multiple forms of assertion, such as J_ASSERT().
> J_ASEERT is provided for the jbd module, which is a public module.

(typo) "J_ASSERT()"

> Maybe we should use custom ASSERT() like other file systems, such as
> xfs, which would be better.

My one minor complaint is that "ASSERT()" is a very generic name and is
likely to cause conflicts with ASSERT in other headers.  That said, I
also see many other filesystems using their own ASSERT() macro, so I
guess they are all in private headers only?

Some minor comments/questions below, but not worth changing the patch
unless you think they are important...

You can add:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb,
> 	struct ext4_sb_info *sbi = EXT4_SB(sb);
> 	ext4_fsblk_t start, tmp;
> 
> -	J_ASSERT_BH(bh, buffer_locked(bh));
> +	ASSERT(buffer_locked(bh));

I thought J_ASSERT_BH() did something useful, but J_ASSERT_BH() just maps
to J_ASSERT() internally anyway.

> +#define ASSERT(assert)							\
> +do {									\
> +	if (unlikely(!(assert))) {					\
> +		printk(KERN_EMERG					\
> +		       "Assertion failure in %s() at %s:%d: \"%s\"\n",	\

(style) better to use single quotes '%s' to avoid the need to escape \".

Cheers, Andreas
Chunguang Xu Oct. 20, 2020, 5:03 a.m. UTC | #2
Thanks for your reply.

Andreas Dilger wrote on 2020/10/20 11:37:
> On Oct 19, 2020, at 3:02 AM, Chunguang Xu <brookxu.cn@gmail.com> wrote:
>>
>> There are currently multiple forms of assertion, such as J_ASSERT().
>> J_ASEERT is provided for the jbd module, which is a public module.
> 
> (typo) "J_ASSERT()"
Thanks, I  will Fixed that.

>> Maybe we should use custom ASSERT() like other file systems, such as
>> xfs, which would be better.
> 
> My one minor complaint is that "ASSERT()" is a very generic name and is
> likely to cause conflicts with ASSERT in other headers.  That said, I
> also see many other filesystems using their own ASSERT() macro, so I
> guess they are all in private headers only?
I also thought about this before, but even if we define it in a private
header file, because we still include several header files in a certain
file, it seems that the conflict cannot be resolved. However, maybe it
is safest to use a name with ext4 prefix. I will try to fix it in next
version. Thanks.

> Some minor comments/questions below, but not worth changing the patch
> unless you think they are important...
> 
> You can add:
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> 
>> @@ -185,7 +185,7 @@ static int ext4_init_block_bitmap(struct super_block *sb,
>> 	struct ext4_sb_info *sbi = EXT4_SB(sb);
>> 	ext4_fsblk_t start, tmp;
>>
>> -	J_ASSERT_BH(bh, buffer_locked(bh));
>> +	ASSERT(buffer_locked(bh));
> 
> I thought J_ASSERT_BH() did something useful, but J_ASSERT_BH() just maps
> to J_ASSERT() internally anyway.
> 
>> +#define ASSERT(assert)							\
>> +do {									\
>> +	if (unlikely(!(assert))) {					\
>> +		printk(KERN_EMERG					\
>> +		       "Assertion failure in %s() at %s:%d: \"%s\"\n",	\
> 
> (style) better to use single quotes '%s' to avoid the need to escape \".
Thanks, this is a good suggestion, I will fix it next version.

> Cheers, Andreas
> 
> 
> 
> 
>
diff mbox series

Patch

diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
index dea738b..386cfc3 100644
--- a/fs/ext4/balloc.c
+++ b/fs/ext4/balloc.c
@@ -185,7 +185,7 @@  static int ext4_init_block_bitmap(struct super_block *sb,
 	struct ext4_sb_info *sbi = EXT4_SB(sb);
 	ext4_fsblk_t start, tmp;
 
-	J_ASSERT_BH(bh, buffer_locked(bh));
+	ASSERT(buffer_locked(bh));
 
 	/* If checksum is bad mark all blocks used to prevent allocation
 	 * essentially implementing a per-group read-only flag. */
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 250e905..512f833 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -98,6 +98,16 @@ 
 #define ext_debug(ino, fmt, ...)	no_printk(fmt, ##__VA_ARGS__)
 #endif
 
+#define ASSERT(assert)							\
+do {									\
+	if (unlikely(!(assert))) {					\
+		printk(KERN_EMERG					\
+		       "Assertion failure in %s() at %s:%d: \"%s\"\n",	\
+		       __func__, __FILE__, __LINE__, #assert);		\
+		BUG();							\
+	}								\
+} while (0)
+
 /* data type for block offset of block group */
 typedef int ext4_grpblk_t;
 
diff --git a/fs/ext4/fsync.c b/fs/ext4/fsync.c
index 6476994..f9e33c7 100644
--- a/fs/ext4/fsync.c
+++ b/fs/ext4/fsync.c
@@ -136,7 +136,7 @@  int ext4_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
 	if (unlikely(ext4_forced_shutdown(sbi)))
 		return -EIO;
 
-	J_ASSERT(ext4_journal_current_handle() == NULL);
+	ASSERT(ext4_journal_current_handle() == NULL);
 
 	trace_ext4_sync_file_enter(file, datasync);
 
diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 05efa682..1223a18 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -534,8 +534,8 @@  int ext4_ind_map_blocks(handle_t *handle, struct inode *inode,
 	ext4_fsblk_t first_block = 0;
 
 	trace_ext4_ind_map_blocks_enter(inode, map->m_lblk, map->m_len, flags);
-	J_ASSERT(!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)));
-	J_ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0);
+	ASSERT(!(ext4_test_inode_flag(inode, EXT4_INODE_EXTENTS)));
+	ASSERT(handle != NULL || (flags & EXT4_GET_BLOCKS_CREATE) == 0);
 	depth = ext4_block_to_path(inode, map->m_lblk, offsets,
 				   &blocks_to_boundary);
 
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 09096fe..8113898 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -825,7 +825,7 @@  struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
 	int create = map_flags & EXT4_GET_BLOCKS_CREATE;
 	int err;
 
-	J_ASSERT(handle != NULL || create == 0);
+	ASSERT(handle != NULL || create == 0);
 
 	map.m_lblk = block;
 	map.m_len = 1;
@@ -840,8 +840,8 @@  struct buffer_head *ext4_getblk(handle_t *handle, struct inode *inode,
 	if (unlikely(!bh))
 		return ERR_PTR(-ENOMEM);
 	if (map.m_flags & EXT4_MAP_NEW) {
-		J_ASSERT(create != 0);
-		J_ASSERT(handle != NULL);
+		ASSERT(create != 0);
+		ASSERT(handle != NULL);
 
 		/*
 		 * Now that we do not always journal data, we should
diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
index b735372..1cb9424 100644
--- a/fs/ext4/namei.c
+++ b/fs/ext4/namei.c
@@ -182,10 +182,6 @@  static struct buffer_head *__ext4_read_dirblock(struct inode *inode,
 	return bh;
 }
 
-#ifndef assert
-#define assert(test) J_ASSERT(test)
-#endif
-
 #ifdef DX_DEBUG
 #define dxtrace(command) command
 #else
@@ -849,7 +845,7 @@  struct stats dx_show_entries(struct dx_hash_info *hinfo, struct inode *dir,
 					break;
 				}
 			}
-			assert (at == p - 1);
+			ASSERT(at == p - 1);
 		}
 
 		at = p - 1;
@@ -1265,8 +1261,8 @@  static void dx_insert_block(struct dx_frame *frame, u32 hash, ext4_lblk_t block)
 	struct dx_entry *old = frame->at, *new = old + 1;
 	int count = dx_get_count(entries);
 
-	assert(count < dx_get_limit(entries));
-	assert(old < entries + count);
+	ASSERT(count < dx_get_limit(entries));
+	ASSERT(old < entries + count);
 	memmove(new + 1, new, (char *)(entries + count) - (char *)(new));
 	dx_set_hash(new, hash);
 	dx_set_block(new, block);
@@ -2959,7 +2955,7 @@  int ext4_orphan_add(handle_t *handle, struct inode *inode)
 	 * hold i_mutex, or the inode can not be referenced from outside,
 	 * so i_nlink should not be bumped due to race
 	 */
-	J_ASSERT((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
+	ASSERT((S_ISREG(inode->i_mode) || S_ISDIR(inode->i_mode) ||
 		  S_ISLNK(inode->i_mode)) || inode->i_nlink == 0);
 
 	BUFFER_TRACE(sbi->s_sbh, "get_write_access");
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 9d01318..8aa163a 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -1251,7 +1251,7 @@  static void ext4_put_super(struct super_block *sb)
 	 * in-memory list had better be clean by this point. */
 	if (!list_empty(&sbi->s_orphan))
 		dump_orphan_list(sb, sbi);
-	J_ASSERT(list_empty(&sbi->s_orphan));
+	ASSERT(list_empty(&sbi->s_orphan));
 
 	sync_blockdev(sb->s_bdev);
 	invalidate_bdev(sb->s_bdev);