ext4: add ext4_sb_bread() to disambiguate ENOMEM cases

PATCH In review Theodore Ts'o 4 participants 8 messages Nov 18, 2018 – Nov 23, 2018 ext4 file system

Share & tools

Theodore Ts'o

author maintainer

2018-11-18 09:54 UTC #0 lore raw

PATCH 1/1 ext4: add ext4_sb_bread() to disambiguate ENOMEM cases

Today, when sb_bread() returns NULL, this can either be because of an I/O error or because the system failed to allocate the buffer. Since it's an old interface, changing would require changing many call sites.

So instead we create our own ext4_sb_bread(), which also allows us to set the REQ_META flag.

Also fixed a problem in the xattr code where a NULL return in a function could also mean that the xattr was not found, which could lead to the wrong error getting returned to userspace.

Fixes ac27a0ec112a ("ext4: initial copy of files from ext3")

Cc [email protected]

Signed-off-by Theodore Ts'o <[email protected]>

---
 fs/ext4/ext4.h    |  2 ++
 fs/ext4/migrate.c | 36 +++++++++++-----------
 fs/ext4/resize.c  | 72 ++++++++++++++++++++++----------------------
 fs/ext4/super.c   | 23 ++++++++++++++
 fs/ext4/xattr.c   | 76 ++++++++++++++++++++++-------------------------
 5 files changed, 115 insertions(+), 94 deletions(-)

fs/ext4/ext4.h +2 -0

index 3f89d0ab08fc..863549a85a34 100644
@@ -2538,6 +2538,8 @@ extern int ext4_group_extend(struct super_block *sb,
 extern int ext4_resize_fs(struct super_block *sb, ext4_fsblk_t n_blocks_count);
 
 /* super.c */
+extern struct buffer_head *ext4_sb_bread(struct super_block *sb,
+					 sector_t block);
 extern int ext4_seq_options_show(struct seq_file *seq, void *offset);
 extern int ext4_calculate_overhead(struct super_block *sb);
 extern void ext4_superblock_csum_set(struct super_block *sb);

fs/ext4/migrate.c +18 ~~-18~~

index 61a9d1927817..f7eda2c89079 100644
@@ -116,9 +116,9 @@ static int update_ind_extent_range(handle_t *handle, struct inode *inode,
 	int i, retval = 0;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, pblock);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, pblock);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	i_data = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -145,9 +145,9 @@ static int update_dind_extent_range(handle_t *handle, struct inode *inode,
 	int i, retval = 0;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, pblock);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, pblock);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	i_data = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -175,9 +175,9 @@ static int update_tind_extent_range(handle_t *handle, struct inode *inode,
 	int i, retval = 0;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, pblock);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, pblock);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	i_data = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -224,9 +224,9 @@ static int free_dind_blocks(handle_t *handle,
 	struct buffer_head *bh;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, le32_to_cpu(i_data));
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, le32_to_cpu(i_data));
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	tmp_idata = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -254,9 +254,9 @@ static int free_tind_blocks(handle_t *handle,
 	struct buffer_head *bh;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, le32_to_cpu(i_data));
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, le32_to_cpu(i_data));
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	tmp_idata = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -382,9 +382,9 @@ static int free_ext_idx(handle_t *handle, struct inode *inode,
 	struct ext4_extent_header *eh;
 
 	block = ext4_idx_pblock(ix);
-	bh = sb_bread(inode->i_sb, block);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, block);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	eh = (struct ext4_extent_header *)bh->b_data;
 	if (eh->eh_depth != 0) {

fs/ext4/resize.c +36 ~~-36~~

167 lines not on this page · raw message

fs/ext4/super.c +23 -0

index 53ff6c2a26ed..c09822001b25 100644
@@ -140,6 +140,29 @@ MODULE_ALIAS_FS("ext3");
 MODULE_ALIAS("ext3");
 #define IS_EXT3_SB(sb) ((sb)->s_bdev->bd_holder == &ext3_fs_type)
 
+/*
+ * This works like sb_bread() except it uses ERR_PTR for error
+ * returns.  Currently with sb_bread it's impossible to distinguish
+ * between ENOMEM and EIO situations (since both result in a NULL
+ * return.
+ */
+struct buffer_head *
+ext4_sb_bread(struct super_block *sb, sector_t block)
+{
+	struct buffer_head *bh = sb_getblk(sb, block);
+
+	if (bh == NULL)
+		return ERR_PTR(-ENOMEM);
+	if (buffer_uptodate(bh))
+		return bh;
+	ll_rw_block(REQ_OP_READ, REQ_META | REQ_PRIO, 1, &bh);
+	wait_on_buffer(bh);
+	if (buffer_uptodate(bh))
+		return bh;
+	put_bh(bh);
+	return ERR_PTR(-EIO);
+}
+
 static int ext4_verify_csum_type(struct super_block *sb,
 				 struct ext4_super_block *es)
 {

fs/ext4/xattr.c +36 ~~-40~~

152 lines not on this page · raw message

Signature · 1 line

2.19.1

kbuild test robot

build bot

2018-11-20 11:41 UTC #1 lore raw

I love your patch! Perhaps something to improve:

Hi Theodore,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ext4/dev]
[also build test WARNING on v4.20-rc3 next-20181119]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-add-ext4_sb_bread-to-disambiguate-ENOMEM-cases/20181120-070629
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev
config: ia64-defconfig (attached as .config)
compiler: ia64-linux-gcc (GCC) 8.1.0

reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
GCC_VERSION=8.1.0 make.cross ARCH=ia64

All warnings (new ones prefixed by >>):

In file included from include/uapi/linux/posix_types.h:5,

                    from include/uapi/linux/types.h:14,
                    from include/linux/compiler.h:169,
                    from include/linux/init.h:5,
                    from fs//ext4/xattr.c:54:
   fs//ext4/xattr.c: In function 'ext4_xattr_block_list':
>> include/linux/stddef.h:8:14: warning: returning 'void *' from a function with return type 'int' makes integer from pointer without a cast [-Wint-conversion]
    #define NULL ((void *)0)
                 ^
   fs//ext4/xattr.c:699:10: note: in expansion of macro 'NULL'
      return NULL;
             ^~~~
--
   In file included from include/uapi/linux/posix_types.h:5,
                    from include/uapi/linux/types.h:14,
                    from include/linux/compiler.h:169,
                    from include/linux/init.h:5,
                    from fs/ext4/xattr.c:54:
   fs/ext4/xattr.c: In function 'ext4_xattr_block_list':
>> include/linux/stddef.h:8:14: warning: returning 'void *' from a function with return type 'int' makes integer from pointer without a cast [-Wint-conversion]
    #define NULL ((void *)0)
                 ^
   fs/ext4/xattr.c:699:10: note: in expansion of macro 'NULL'
      return NULL;
             ^~~~

vim +8 include/linux/stddef.h

^1da177e Linus Torvalds   2005-04-16  6  
^1da177e Linus Torvalds   2005-04-16  7  #undef NULL
^1da177e Linus Torvalds   2005-04-16 @8  #define NULL ((void *)0)

6e218287 Richard Knutsson 2006-09-30 9

:::::: The code at line 8 was first introduced by commit :::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <[email protected]> :::::: CC: Linus Torvalds <[email protected]>

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

kbuild test robot

build bot

2018-11-21 00:06 UTC #2 lore raw

[PATCH] ext4: fix odd_ptr_err.cocci warnings

In reply to Theodore Ts'o

fs/ext4/resize.c:999:6-12: inconsistent IS_ERR and PTR_ERR on line 1000.

From: kbuild test robot <[email protected]>

fs/ext4/resize.c:999:6-12: inconsistent IS_ERR and PTR_ERR on line 1000.

PTR_ERR should access the value just tested by IS_ERR

Semantic patch information:
There can be false positives in the patch case, where it is the call to IS_ERR that is wrong.

Generated by: scripts/coccinelle/tests/odd_ptr_err.cocci

Fixes 0212baa67119 ("ext4: add ext4_sb_bread() to disambiguate ENOMEM cases")

CC Theodore Ts'o <[email protected]>

Signed-off-by kbuild test robot <[email protected]>

---

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-add-ext4_sb_bread-to-disambiguate-ENOMEM-cases/20181120-070629
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev

Please take the patch only if it's a positive warning. Thanks!

 resize.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

fs/ext4/resize.c +1 -1

@@ -997,7 +997,7 @@ static int reserve_backup_gdb(handle_t *
 		}
 		primary[res] = ext4_sb_bread(sb, blk);
 		if (IS_ERR(primary[res])) {
-			err = PTR_ERR(res);
+			err = PTR_ERR(primary[res]);
 			primary[res] = NULL;
 			goto exit_bh;
 		}

kbuild test robot

build bot

2018-11-21 00:06 UTC #3 lore raw

In reply to Theodore Ts'o

I love your patch! Perhaps something to improve:

Hi Theodore,

I love your patch! Perhaps something to improve:

[auto build test WARNING on ext4/dev]
[also build test WARNING on v4.20-rc3 next-20181120]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Theodore-Ts-o/ext4-add-ext4_sb_bread-to-disambiguate-ENOMEM-cases/20181120-070629
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4.git dev

coccinelle warnings: (new ones prefixed by >>)

>> fs/ext4/resize.c:1000:9-16: ERROR: PTR_ERR applied after initialization to constant on line 989
--
>> fs/ext4/resize.c:999:6-12: inconsistent IS_ERR and PTR_ERR on line 1000.

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation

Jan Kara

reviewer

2018-11-22 23:01 UTC #4 lore raw

In reply to Theodore Ts'o

Quoted 15 lines · Theodore Ts'o

Today, when sb_bread() returns NULL, this can either be because of an I/O error or because the system failed to allocate the buffer. Since it's an old interface, changing would require changing many call sites.
So instead we create our own ext4_sb_bread(), which also allows us to set the REQ_META flag.
Also fixed a problem in the xattr code where a NULL return in a function could also mean that the xattr was not found, which could lead to the wrong error getting returned to userspace.
Fixes ac27a0ec112a ("ext4: initial copy of files from ext3")
Cc [email protected]
Signed-off-by Theodore Ts'o <[email protected]>

...

Quoted 6 lines

fs/ext4/super.c +6 -0

+/*
+ * This works like sb_bread() except it uses ERR_PTR for error
+ * returns.  Currently with sb_bread it's impossible to distinguish
+ * between ENOMEM and EIO situations (since both result in a NULL
+ * return.
+ */

fs/ext4/super.c MODULE_ALIAS_FS("ext3"); · line 158

Quoted 10 lines

fs/ext4/super.c +10 -0

+struct buffer_head *
+ext4_sb_bread(struct super_block *sb, sector_t block)
+{
+	struct buffer_head *bh = sb_getblk(sb, block);
+
+	if (bh == NULL)
+		return ERR_PTR(-ENOMEM);
+	if (buffer_uptodate(bh))
+		return bh;
+	ll_rw_block(REQ_OP_READ, REQ_META | REQ_PRIO, 1, &bh);

Is there a reason for REQ_PRIO? I'm not sure all REQ_META reads are really a priority ones...

Quoted 10 lines

fs/ext4/super.c +7 -0

+	wait_on_buffer(bh);
+	if (buffer_uptodate(bh))
+		return bh;
+	put_bh(bh);
+	return ERR_PTR(-EIO);
+}
+
 static int ext4_verify_csum_type(struct super_block *sb,
 				 struct ext4_super_block *es)
 {

...

fs/ext4/xattr.c ext4_xattr_block_list(struct dentry *dentry, char *buffer, size_t buffer_size) · line 699

Quoted 8 lines

fs/ext4/xattr.c +1 -2

@@ -696,26 +695,23 @@ ext4_xattr_block_list(struct dentry *dentry, char *buffer, size_t buffer_size)
 	ea_idebug(inode, "buffer=%p, buffer_size=%ld",
 		  buffer, (long)buffer_size);
 
-	error = 0;
 	if (!EXT4_I(inode)->i_file_acl)
-		goto cleanup;
+		return NULL;

NULL looks wrong here - should be 0 I guess...

Otherwise the patch looks good to me.

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR

Theodore Y. Ts'o

author maintainer

2018-11-23 07:53 UTC #5 lore raw

Quoted 3 lines · Jan Kara

Is there a reason for REQ_PRIO? I'm not sure all REQ_META reads are really a priority ones...

Hmm, good question. With the exception of readahead, most reads will be blocking some process. We are currently using REQ_PRIO for all directory block reads. The ext4_sb_read() function gets used for resizing, indirect block map to extent tree migration, and extended attribute reads. The last is the most common, and arguably the most justifiable to be REQ_PRIO. (Of course my understanding is that the block layer is ignoring REQ_PRIO, so this is mostly academic...)

So I think what I'll do is this. I'll add a parameter to ext4_sb_read() so each caller can use use REQ_PRIO. REQ_PRIO will be used from xattr.c, but not from fs/ext4/migrate.c and fs/ext4/resize.c.

- Ted

Theodore Ts'o

author maintainer

2018-11-23 08:59 UTC #6 lore raw

PATCH 1/1 ext4: add ext4_sb_bread() to disambiguate ENOMEM cases

So instead we create our own ext4_sb_bread(), which also allows us to set the REQ_META flag.

Also fixed a problem in the xattr code where a NULL return in a function could also mean that the xattr was not found, which could lead to the wrong error getting returned to userspace.

Fixes ac27a0ec112a ("ext4: initial copy of files from ext3")

Cc [email protected]

Signed-off-by Theodore Ts'o <[email protected]>

---
 fs/ext4/ext4.h    |  2 ++
 fs/ext4/migrate.c | 36 +++++++++++-----------
 fs/ext4/resize.c  | 72 ++++++++++++++++++++++----------------------
 fs/ext4/super.c   | 23 ++++++++++++++
 fs/ext4/xattr.c   | 76 ++++++++++++++++++++++-------------------------
 5 files changed, 115 insertions(+), 94 deletions(-)

fs/ext4/ext4.h +2 -0

index 3f89d0ab08fc..b4621277e259 100644
@@ -2538,6 +2538,8 @@ extern int ext4_group_extend(struct super_block *sb,
 extern int ext4_resize_fs(struct super_block *sb, ext4_fsblk_t n_blocks_count);
 
 /* super.c */
+extern struct buffer_head *ext4_sb_bread(struct super_block *sb,
+					 sector_t block, int op_flags);
 extern int ext4_seq_options_show(struct seq_file *seq, void *offset);
 extern int ext4_calculate_overhead(struct super_block *sb);
 extern void ext4_superblock_csum_set(struct super_block *sb);

fs/ext4/migrate.c +18 ~~-18~~

index 61a9d1927817..a98bfca9c463 100644
@@ -116,9 +116,9 @@ static int update_ind_extent_range(handle_t *handle, struct inode *inode,
 	int i, retval = 0;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, pblock);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, pblock, 0);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	i_data = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -145,9 +145,9 @@ static int update_dind_extent_range(handle_t *handle, struct inode *inode,
 	int i, retval = 0;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, pblock);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, pblock, 0);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	i_data = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -175,9 +175,9 @@ static int update_tind_extent_range(handle_t *handle, struct inode *inode,
 	int i, retval = 0;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, pblock);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, pblock, 0);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	i_data = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -224,9 +224,9 @@ static int free_dind_blocks(handle_t *handle,
 	struct buffer_head *bh;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, le32_to_cpu(i_data));
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, le32_to_cpu(i_data), 0);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	tmp_idata = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -254,9 +254,9 @@ static int free_tind_blocks(handle_t *handle,
 	struct buffer_head *bh;
 	unsigned long max_entries = inode->i_sb->s_blocksize >> 2;
 
-	bh = sb_bread(inode->i_sb, le32_to_cpu(i_data));
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, le32_to_cpu(i_data), 0);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	tmp_idata = (__le32 *)bh->b_data;
 	for (i = 0; i < max_entries; i++) {

@@ -382,9 +382,9 @@ static int free_ext_idx(handle_t *handle, struct inode *inode,
 	struct ext4_extent_header *eh;
 
 	block = ext4_idx_pblock(ix);
-	bh = sb_bread(inode->i_sb, block);
-	if (!bh)
-		return -EIO;
+	bh = ext4_sb_bread(inode->i_sb, block, 0);
+	if (IS_ERR(bh))
+		return PTR_ERR(bh);
 
 	eh = (struct ext4_extent_header *)bh->b_data;
 	if (eh->eh_depth != 0) {

fs/ext4/resize.c +36 ~~-36~~

167 lines not on this page · raw message

fs/ext4/super.c +23 -0

index 53ff6c2a26ed..361624460431 100644
@@ -140,6 +140,29 @@ MODULE_ALIAS_FS("ext3");
 MODULE_ALIAS("ext3");
 #define IS_EXT3_SB(sb) ((sb)->s_bdev->bd_holder == &ext3_fs_type)
 
+/*
+ * This works like sb_bread() except it uses ERR_PTR for error
+ * returns.  Currently with sb_bread it's impossible to distinguish
+ * between ENOMEM and EIO situations (since both result in a NULL
+ * return.
+ */
+struct buffer_head *
+ext4_sb_bread(struct super_block *sb, sector_t block, int op_flags)
+{
+	struct buffer_head *bh = sb_getblk(sb, block);
+
+	if (bh == NULL)
+		return ERR_PTR(-ENOMEM);
+	if (buffer_uptodate(bh))
+		return bh;
+	ll_rw_block(REQ_OP_READ, REQ_META | op_flags, 1, &bh);
+	wait_on_buffer(bh);
+	if (buffer_uptodate(bh))
+		return bh;
+	put_bh(bh);
+	return ERR_PTR(-EIO);
+}
+
 static int ext4_verify_csum_type(struct super_block *sb,
 				 struct ext4_super_block *es)
 {

fs/ext4/xattr.c +36 ~~-40~~

152 lines not on this page · raw message

Signature · 1 line

2.19.1

Jan Kara

reviewer

2018-11-23 22:59 UTC #7 lore raw

In reply to Theodore Y. Ts'o

Quoted 5 lines · Theodore Y. Ts'o

Quoted 3 lines · Jan Kara
Is there a reason for REQ_PRIO? I'm not sure all REQ_META reads are really a priority ones...

Quoted 7 lines

Hmm, good question. With the exception of readahead, most reads will be blocking some process. We are currently using REQ_PRIO for all directory block reads. The ext4_sb_read() function gets used for resizing, indirect block map to extent tree migration, and extended attribute reads. The last is the most common, and arguably the most justifiable to be REQ_PRIO. (Of course my understanding is that the block layer is ignoring REQ_PRIO, so this is mostly academic...)

Agreed that this is currently academic but AFAIR some blk-mq schedulers might be improved to consider REQ_PRIO again.

Quoted 4 lines

So I think what I'll do is this. I'll add a parameter to ext4_sb_read() so each caller can use use REQ_PRIO. REQ_PRIO will be used from xattr.c, but not from fs/ext4/migrate.c and fs/ext4/resize.c.

Sounds good. Thanks!

Honza

Signature · 2 lines

Jan Kara <[email protected]>
SUSE Labs, CR