From d5ac77730516028f3ceda825abefac9a1153b138 Mon Sep 17 00:00:00 2001 From: Guo Chao Date: Mon, 28 Jan 2013 21:23:24 -0500 Subject: ext4: release buffer when checksum failed Commit b0336e8d (ext4: calculate and verify checksums of directory leaf blocks) and commit dbe89444 (ext4: Calculate and verify checksums for htree nodes) forget to release buffer when checksum failed, at some places. Signed-off-by: Guo Chao Signed-off-by: "Theodore Ts'o" Reviewed-by: Darrick J. Wong --- fs/ext4/namei.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index f9ed946..99813db 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -837,6 +837,7 @@ static int ext4_htree_next_block(struct inode *dir, __u32 hash, !ext4_dx_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) { ext4_warning(dir->i_sb, "Node failed checksum"); + brelse(bh); return -EIO; } set_buffer_verified(bh); @@ -877,8 +878,11 @@ static int htree_dirblock_to_tree(struct file *dir_file, } if (!buffer_verified(bh) && - !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) + !ext4_dirent_csum_verify(dir, + (struct ext4_dir_entry *)bh->b_data)) { + brelse(bh); return -EIO; + } set_buffer_verified(bh); de = (struct ext4_dir_entry_2 *) bh->b_data; @@ -1929,8 +1933,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, } if (!buffer_verified(bh) && !ext4_dirent_csum_verify(dir, - (struct ext4_dir_entry *)bh->b_data)) + (struct ext4_dir_entry *)bh->b_data)) { + brelse(bh); return -EIO; + } set_buffer_verified(bh); retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh); if (retval != -ENOSPC) { @@ -2492,6 +2498,7 @@ static int empty_dir(struct inode *inode) (struct ext4_dir_entry *)bh->b_data)) { EXT4_ERROR_INODE(inode, "checksum error reading directory " "lblock 0"); + brelse(bh); return -EIO; } set_buffer_verified(bh); @@ -2536,6 +2543,7 @@ static int empty_dir(struct inode *inode) (struct ext4_dir_entry *)bh->b_data)) { EXT4_ERROR_INODE(inode, "checksum error " "reading directory lblock 0"); + brelse(bh); return -EIO; } set_buffer_verified(bh); -- cgit v1.1 From 2bbbee2a68a726deeac7da5ae7dd79b00301d6fd Mon Sep 17 00:00:00 2001 From: Guo Chao Date: Mon, 28 Jan 2013 21:26:44 -0500 Subject: ext4: remove unused variable in add_dirent_to_buf() After commit 978fef9 (create __ext4_insert_dentry for dir entry insertion), 'reclen' is not used anymore. Signed-off-by: Guo Chao Signed-off-by: "Theodore Ts'o" Reviewed-by: Darrick J. Wong --- fs/ext4/namei.c | 2 -- 1 file changed, 2 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 99813db..8bc01a3 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -1703,7 +1703,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry, const char *name = dentry->d_name.name; int namelen = dentry->d_name.len; unsigned int blocksize = dir->i_sb->s_blocksize; - unsigned short reclen; int csum_size = 0; int err; @@ -1711,7 +1710,6 @@ static int add_dirent_to_buf(handle_t *handle, struct dentry *dentry, EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) csum_size = sizeof(struct ext4_dir_entry_tail); - reclen = EXT4_DIR_REC_LEN(namelen); if (!de) { err = ext4_find_dest_de(dir, inode, bh, bh->b_data, blocksize - csum_size, -- cgit v1.1 From 41be871f747f64e076b09a68ae82a643e2ffb215 Mon Sep 17 00:00:00 2001 From: Guo Chao Date: Mon, 28 Jan 2013 21:33:28 -0500 Subject: ext4: remove useless assignment in dx_probe() Signed-off-by: Guo Chao Signed-off-by: "Theodore Ts'o" --- fs/ext4/namei.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 8bc01a3..f4b9587 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -714,7 +714,7 @@ dx_probe(const struct qstr *d_name, struct inode *dir, *err = ERR_BAD_DX_DIR; goto fail2; } - at = entries = ((struct dx_node *) bh->b_data)->entries; + entries = ((struct dx_node *) bh->b_data)->entries; if (!buffer_verified(bh) && !ext4_dx_csum_verify(dir, -- cgit v1.1 From b1deefc99e668348f7c785c6ece5f6ff4c6d8b5c Mon Sep 17 00:00:00 2001 From: Guo Chao Date: Mon, 28 Jan 2013 21:41:02 -0500 Subject: ext4: remove unnecessary NULL pointer check brelse() and ext4_journal_force_commit() are both inlined and able to handle NULL. Signed-off-by: Guo Chao Reviewed-by: Darrick J. Wong Signed-off-by: "Theodore Ts'o" --- fs/ext4/namei.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index f4b9587..34ed624d 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2110,8 +2110,7 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry, journal_error: ext4_std_error(dir->i_sb, err); cleanup: - if (bh) - brelse(bh); + brelse(bh); dx_release(frames); return err; } -- cgit v1.1 From 9924a92a8c217576bd2a2b1bbbb854462f1a00ae Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Fri, 8 Feb 2013 21:59:22 -0500 Subject: ext4: pass context information to jbd2__journal_start() So we can better understand what bits of ext4 are responsible for long-running jbd2 handles, use jbd2__journal_start() so we can pass context information for logging purposes. The recommended way for finding the longer-running handles is: T=/sys/kernel/debug/tracing EVENT=$T/events/jbd2/jbd2_handle_stats echo "interval > 5" > $EVENT/filter echo 1 > $EVENT/enable ./run-my-fs-benchmark cat $T/trace > /tmp/problem-handles This will list handles that were active for longer than 20ms. Having longer-running handles is bad, because a commit started at the wrong time could stall for those 20+ milliseconds, which could delay an fsync() or an O_SYNC operation. Here is an example line from the trace file describing a handle which lived on for 311 jiffies, or over 1.2 seconds: postmark-2917 [000] .... 196.435786: jbd2_handle_stats: dev 254,32 tid 570 type 2 line_no 2541 interval 311 sync 0 requested_blocks 1 dirtied_blocks 0 Signed-off-by: "Theodore Ts'o" --- fs/ext4/namei.c | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 34ed624d..5184103 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2262,9 +2262,10 @@ static int ext4_create(struct inode *dir, struct dentry *dentry, umode_t mode, dquot_initialize(dir); retry: - handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + handle = ext4_journal_start(dir, EXT4_HT_DIR, + (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb))); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2298,9 +2299,10 @@ static int ext4_mknod(struct inode *dir, struct dentry *dentry, dquot_initialize(dir); retry: - handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + handle = ext4_journal_start(dir, EXT4_HT_DIR, + (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb))); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2414,9 +2416,10 @@ static int ext4_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) dquot_initialize(dir); retry: - handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); + handle = ext4_journal_start(dir, EXT4_HT_DIR, + (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb))); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2729,7 +2732,8 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry) dquot_initialize(dir); dquot_initialize(dentry->d_inode); - handle = ext4_journal_start(dir, EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); + handle = ext4_journal_start(dir, EXT4_HT_DIR, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2791,7 +2795,8 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry) dquot_initialize(dir); dquot_initialize(dentry->d_inode); - handle = ext4_journal_start(dir, EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); + handle = ext4_journal_start(dir, EXT4_HT_DIR, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2870,7 +2875,7 @@ static int ext4_symlink(struct inode *dir, EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb); } retry: - handle = ext4_journal_start(dir, credits); + handle = ext4_journal_start(dir, EXT4_HT_DIR, credits); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -2908,7 +2913,7 @@ retry: * Now inode is being linked into dir (EXT4_DATA_TRANS_BLOCKS * + EXT4_INDEX_EXTRA_TRANS_BLOCKS), inode is also modified */ - handle = ext4_journal_start(dir, + handle = ext4_journal_start(dir, EXT4_HT_DIR, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 1); if (IS_ERR(handle)) { @@ -2955,8 +2960,9 @@ static int ext4_link(struct dentry *old_dentry, dquot_initialize(dir); retry: - handle = ext4_journal_start(dir, EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS); + handle = ext4_journal_start(dir, EXT4_HT_DIR, + (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS)); if (IS_ERR(handle)) return PTR_ERR(handle); @@ -3039,9 +3045,9 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, * in separate transaction */ if (new_dentry->d_inode) dquot_initialize(new_dentry->d_inode); - handle = ext4_journal_start(old_dir, 2 * - EXT4_DATA_TRANS_BLOCKS(old_dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2); + handle = ext4_journal_start(old_dir, EXT4_HT_DIR, + (2 * EXT4_DATA_TRANS_BLOCKS(old_dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 2)); if (IS_ERR(handle)) return PTR_ERR(handle); -- cgit v1.1 From 931b68649d31b6b52608110f34856f8eb77adb36 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Sat, 9 Feb 2013 09:43:39 -0500 Subject: ext4: start handle at the last possible moment in ext4_unlink() Don't start the jbd2 transaction handle until after the directory entry has been found, to minimize the amount of time that a handle is held active. Signed-off-by: "Theodore Ts'o" Reviewed-by: Jan Kara --- fs/ext4/namei.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 5184103..4a27069 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2787,7 +2787,7 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry) struct inode *inode; struct buffer_head *bh; struct ext4_dir_entry_2 *de; - handle_t *handle; + handle_t *handle = NULL; trace_ext4_unlink_enter(dir, dentry); /* Initialize quotas before so that eventual writes go @@ -2795,14 +2795,6 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry) dquot_initialize(dir); dquot_initialize(dentry->d_inode); - handle = ext4_journal_start(dir, EXT4_HT_DIR, - EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); - if (IS_ERR(handle)) - return PTR_ERR(handle); - - if (IS_DIRSYNC(dir)) - ext4_handle_sync(handle); - retval = -ENOENT; bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL); if (!bh) @@ -2814,6 +2806,17 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry) if (le32_to_cpu(de->inode) != inode->i_ino) goto end_unlink; + handle = ext4_journal_start(dir, EXT4_HT_DIR, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); + if (IS_ERR(handle)) { + retval = PTR_ERR(handle); + handle = NULL; + goto end_unlink; + } + + if (IS_DIRSYNC(dir)) + ext4_handle_sync(handle); + if (!inode->i_nlink) { ext4_warning(inode->i_sb, "Deleting nonexistent file (%lu), %d", @@ -2834,8 +2837,9 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry) retval = 0; end_unlink: - ext4_journal_stop(handle); brelse(bh); + if (handle) + ext4_journal_stop(handle); trace_ext4_unlink_exit(dentry, retval); return retval; } -- cgit v1.1 From 8dcfaad244cdfa245cc2b4ddf42cea5fd8b80ece Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Sat, 9 Feb 2013 09:45:11 -0500 Subject: ext4: start handle at the last possible moment in ext4_rmdir() Don't start the jbd2 transaction handle until after the directory entry has been found, to minimize the amount of time that a handle is held active. Signed-off-by: "Theodore Ts'o" Reviewed-by: Jan Kara --- fs/ext4/namei.c | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 4a27069..36a4afd 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2725,26 +2725,18 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry) struct inode *inode; struct buffer_head *bh; struct ext4_dir_entry_2 *de; - handle_t *handle; + handle_t *handle = NULL; /* Initialize quotas before so that eventual writes go in * separate transaction */ dquot_initialize(dir); dquot_initialize(dentry->d_inode); - handle = ext4_journal_start(dir, EXT4_HT_DIR, - EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); - if (IS_ERR(handle)) - return PTR_ERR(handle); - retval = -ENOENT; bh = ext4_find_entry(dir, &dentry->d_name, &de, NULL); if (!bh) goto end_rmdir; - if (IS_DIRSYNC(dir)) - ext4_handle_sync(handle); - inode = dentry->d_inode; retval = -EIO; @@ -2755,6 +2747,17 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry) if (!empty_dir(inode)) goto end_rmdir; + handle = ext4_journal_start(dir, EXT4_HT_DIR, + EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); + if (IS_ERR(handle)) { + retval = PTR_ERR(handle); + handle = NULL; + goto end_rmdir; + } + + if (IS_DIRSYNC(dir)) + ext4_handle_sync(handle); + retval = ext4_delete_entry(handle, dir, de, bh); if (retval) goto end_rmdir; @@ -2776,8 +2779,9 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry) ext4_mark_inode_dirty(handle, dir); end_rmdir: - ext4_journal_stop(handle); brelse(bh); + if (handle) + ext4_journal_stop(handle); return retval; } -- cgit v1.1 From 64044abf05d0842a7fed30e102fa411a744c7d9f Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Sat, 9 Feb 2013 15:06:24 -0500 Subject: ext4: fix the number of credits needed for ext4_unlink() and ext4_rmdir() The ext4_unlink() and ext4_rmdir() don't actually release the blocks associated with the file/directory. This gets done in a separate jbd2 handle called via ext4_evict_inode(). Thus, we don't need to reserve lots of journal credits for the truncate. Note that using too many journal credits is non-optimal because it can leading to the journal transmit getting closed too early, before it is strictly necessary. Signed-off-by: "Theodore Ts'o" Reviewed-by: Jan Kara --- fs/ext4/namei.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 36a4afd..5f3d2b5 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2748,7 +2748,7 @@ static int ext4_rmdir(struct inode *dir, struct dentry *dentry) goto end_rmdir; handle = ext4_journal_start(dir, EXT4_HT_DIR, - EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); + EXT4_DATA_TRANS_BLOCKS(dir->i_sb)); if (IS_ERR(handle)) { retval = PTR_ERR(handle); handle = NULL; @@ -2811,7 +2811,7 @@ static int ext4_unlink(struct inode *dir, struct dentry *dentry) goto end_unlink; handle = ext4_journal_start(dir, EXT4_HT_DIR, - EXT4_DELETE_TRANS_BLOCKS(dir->i_sb)); + EXT4_DATA_TRANS_BLOCKS(dir->i_sb)); if (IS_ERR(handle)) { retval = PTR_ERR(handle); handle = NULL; -- cgit v1.1 From 1139575a927010390c6b38e4215a6d741b056074 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Sat, 9 Feb 2013 16:27:09 -0500 Subject: ext4: start handle at the last possible moment when creating inodes In ext4_{create,mknod,mkdir,symlink}(), don't start the journal handle until the inode has been succesfully allocated. In order to do this, we need to start the handle in the ext4_new_inode(). So create a new variant of this function, ext4_new_inode_start_handle(), so the handle can be created at the last possible minute, before we need to modify the inode allocation bitmap block. Signed-off-by: "Theodore Ts'o" --- fs/ext4/namei.c | 94 +++++++++++++++++++++++++++------------------------------ 1 file changed, 44 insertions(+), 50 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 5f3d2b5..3e1529c 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -2257,30 +2257,28 @@ static int ext4_create(struct inode *dir, struct dentry *dentry, umode_t mode, { handle_t *handle; struct inode *inode; - int err, retries = 0; + int err, credits, retries = 0; dquot_initialize(dir); + credits = (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); retry: - handle = ext4_journal_start(dir, EXT4_HT_DIR, - (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb))); - if (IS_ERR(handle)) - return PTR_ERR(handle); - - if (IS_DIRSYNC(dir)) - ext4_handle_sync(handle); - - inode = ext4_new_inode(handle, dir, mode, &dentry->d_name, 0, NULL); + inode = ext4_new_inode_start_handle(dir, mode, &dentry->d_name, 0, + NULL, EXT4_HT_DIR, credits); + handle = ext4_journal_current_handle(); err = PTR_ERR(inode); if (!IS_ERR(inode)) { inode->i_op = &ext4_file_inode_operations; inode->i_fop = &ext4_file_operations; ext4_set_aops(inode); err = ext4_add_nondir(handle, dentry, inode); + if (!err && IS_DIRSYNC(dir)) + ext4_handle_sync(handle); } - ext4_journal_stop(handle); + if (handle) + ext4_journal_stop(handle); if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries)) goto retry; return err; @@ -2291,32 +2289,30 @@ static int ext4_mknod(struct inode *dir, struct dentry *dentry, { handle_t *handle; struct inode *inode; - int err, retries = 0; + int err, credits, retries = 0; if (!new_valid_dev(rdev)) return -EINVAL; dquot_initialize(dir); + credits = (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); retry: - handle = ext4_journal_start(dir, EXT4_HT_DIR, - (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb))); - if (IS_ERR(handle)) - return PTR_ERR(handle); - - if (IS_DIRSYNC(dir)) - ext4_handle_sync(handle); - - inode = ext4_new_inode(handle, dir, mode, &dentry->d_name, 0, NULL); + inode = ext4_new_inode_start_handle(dir, mode, &dentry->d_name, 0, + NULL, EXT4_HT_DIR, credits); + handle = ext4_journal_current_handle(); err = PTR_ERR(inode); if (!IS_ERR(inode)) { init_special_inode(inode, inode->i_mode, rdev); inode->i_op = &ext4_special_inode_operations; err = ext4_add_nondir(handle, dentry, inode); + if (!err && IS_DIRSYNC(dir)) + ext4_handle_sync(handle); } - ext4_journal_stop(handle); + if (handle) + ext4_journal_stop(handle); if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries)) goto retry; return err; @@ -2408,26 +2404,21 @@ static int ext4_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode) { handle_t *handle; struct inode *inode; - int err, retries = 0; + int err, credits, retries = 0; if (EXT4_DIR_LINK_MAX(dir)) return -EMLINK; dquot_initialize(dir); + credits = (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + + EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + + EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb)); retry: - handle = ext4_journal_start(dir, EXT4_HT_DIR, - (EXT4_DATA_TRANS_BLOCKS(dir->i_sb) + - EXT4_INDEX_EXTRA_TRANS_BLOCKS + 3 + - EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb))); - if (IS_ERR(handle)) - return PTR_ERR(handle); - - if (IS_DIRSYNC(dir)) - ext4_handle_sync(handle); - - inode = ext4_new_inode(handle, dir, S_IFDIR | mode, - &dentry->d_name, 0, NULL); + inode = ext4_new_inode_start_handle(dir, S_IFDIR | mode, + &dentry->d_name, + 0, NULL, EXT4_HT_DIR, credits); + handle = ext4_journal_current_handle(); err = PTR_ERR(inode); if (IS_ERR(inode)) goto out_stop; @@ -2455,8 +2446,12 @@ out_clear_inode: goto out_clear_inode; unlock_new_inode(inode); d_instantiate(dentry, inode); + if (IS_DIRSYNC(dir)) + ext4_handle_sync(handle); + out_stop: - ext4_journal_stop(handle); + if (handle) + ext4_journal_stop(handle); if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries)) goto retry; return err; @@ -2883,15 +2878,10 @@ static int ext4_symlink(struct inode *dir, EXT4_MAXQUOTAS_INIT_BLOCKS(dir->i_sb); } retry: - handle = ext4_journal_start(dir, EXT4_HT_DIR, credits); - if (IS_ERR(handle)) - return PTR_ERR(handle); - - if (IS_DIRSYNC(dir)) - ext4_handle_sync(handle); - - inode = ext4_new_inode(handle, dir, S_IFLNK|S_IRWXUGO, - &dentry->d_name, 0, NULL); + inode = ext4_new_inode_start_handle(dir, S_IFLNK|S_IRWXUGO, + &dentry->d_name, 0, NULL, + EXT4_HT_DIR, credits); + handle = ext4_journal_current_handle(); err = PTR_ERR(inode); if (IS_ERR(inode)) goto out_stop; @@ -2944,8 +2934,12 @@ retry: } EXT4_I(inode)->i_disksize = inode->i_size; err = ext4_add_nondir(handle, dentry, inode); + if (!err && IS_DIRSYNC(dir)) + ext4_handle_sync(handle); + out_stop: - ext4_journal_stop(handle); + if (handle) + ext4_journal_stop(handle); if (err == -ENOSPC && ext4_should_retry_alloc(dir->i_sb, &retries)) goto retry; return err; -- cgit v1.1 From dc6982ff4db1f47da73b1967ef5302d6721e5b95 Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Thu, 14 Feb 2013 23:59:26 -0500 Subject: ext4: refactor code to read directory blocks into ext4_read_dirblock() The code to read in directory blocks and verify their metadata checksums was replicated in ten different places across fs/ext4/namei.c, and the code was buggy in subtle ways in a number of those replicated sites. In some cases, ext4_error() was called with a training newline. In others, in particularly in empty_dir(), it was possible to call ext4_dirent_csum_verify() on an index block, which would trigger false warnings requesting the system adminsitrator to run e2fsck. By refactoring the code, we make the code more readable, as well as shrinking the compiled object file by over 700 bytes and 50 lines of code. Signed-off-by: "Theodore Ts'o" --- fs/ext4/namei.c | 291 +++++++++++++++++++++++--------------------------------- 1 file changed, 119 insertions(+), 172 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 3e1529c..0e28c74 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -83,6 +83,86 @@ static struct buffer_head *ext4_append(handle_t *handle, return bh; } +static int ext4_dx_csum_verify(struct inode *inode, + struct ext4_dir_entry *dirent); + +typedef enum { + EITHER, INDEX, DIRENT +} dirblock_type_t; + +#define ext4_read_dirblock(inode, block, type) \ + __ext4_read_dirblock((inode), (block), (type), __LINE__) + +static struct buffer_head *__ext4_read_dirblock(struct inode *inode, + ext4_lblk_t block, + dirblock_type_t type, + unsigned int line) +{ + struct buffer_head *bh; + struct ext4_dir_entry *dirent; + int err = 0, is_dx_block = 0; + + bh = ext4_bread(NULL, inode, block, 0, &err); + if (!bh) { + if (err == 0) { + ext4_error_inode(inode, __func__, line, block, + "Directory hole found"); + return ERR_PTR(-EIO); + } + __ext4_warning(inode->i_sb, __func__, line, + "error reading directory block " + "(ino %lu, block %lu)", inode->i_ino, + (unsigned long) block); + return ERR_PTR(err); + } + dirent = (struct ext4_dir_entry *) bh->b_data; + /* Determine whether or not we have an index block */ + if (is_dx(inode)) { + if (block == 0) + is_dx_block = 1; + else if (ext4_rec_len_from_disk(dirent->rec_len, + inode->i_sb->s_blocksize) == + inode->i_sb->s_blocksize) + is_dx_block = 1; + } + if (!is_dx_block && type == INDEX) { + ext4_error_inode(inode, __func__, line, block, + "directory leaf block found instead of index block"); + return ERR_PTR(-EIO); + } + if (!EXT4_HAS_RO_COMPAT_FEATURE(inode->i_sb, + EXT4_FEATURE_RO_COMPAT_METADATA_CSUM) || + buffer_verified(bh)) + return bh; + + /* + * An empty leaf block can get mistaken for a index block; for + * this reason, we can only check the index checksum when the + * caller is sure it should be an index block. + */ + if (is_dx_block && type == INDEX) { + if (ext4_dx_csum_verify(inode, dirent)) + set_buffer_verified(bh); + else { + ext4_error_inode(inode, __func__, line, block, + "Directory index failed checksum"); + brelse(bh); + return ERR_PTR(-EIO); + } + } + if (!is_dx_block) { + if (ext4_dirent_csum_verify(inode, dirent)) + set_buffer_verified(bh); + else { + ext4_error_inode(inode, __func__, line, block, + "Directory block failed checksum"); + brelse(bh); + return ERR_PTR(-EIO); + } + } + return bh; +} + #ifndef assert #define assert(test) J_ASSERT(test) #endif @@ -604,9 +684,9 @@ dx_probe(const struct qstr *d_name, struct inode *dir, u32 hash; frame->bh = NULL; - if (!(bh = ext4_bread(NULL, dir, 0, 0, err))) { - if (*err == 0) - *err = ERR_BAD_DX_DIR; + bh = ext4_read_dirblock(dir, 0, INDEX); + if (IS_ERR(bh)) { + *err = PTR_ERR(bh); goto fail; } root = (struct dx_root *) bh->b_data; @@ -643,15 +723,6 @@ dx_probe(const struct qstr *d_name, struct inode *dir, goto fail; } - if (!buffer_verified(bh) && - !ext4_dx_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) { - ext4_warning(dir->i_sb, "Root failed checksum"); - brelse(bh); - *err = ERR_BAD_DX_DIR; - goto fail; - } - set_buffer_verified(bh); - entries = (struct dx_entry *) (((char *)&root->info) + root->info.info_length); @@ -709,23 +780,13 @@ dx_probe(const struct qstr *d_name, struct inode *dir, frame->entries = entries; frame->at = at; if (!indirect--) return frame; - if (!(bh = ext4_bread(NULL, dir, dx_get_block(at), 0, err))) { - if (!(*err)) - *err = ERR_BAD_DX_DIR; + bh = ext4_read_dirblock(dir, dx_get_block(at), INDEX); + if (IS_ERR(bh)) { + *err = PTR_ERR(bh); goto fail2; } entries = ((struct dx_node *) bh->b_data)->entries; - if (!buffer_verified(bh) && - !ext4_dx_csum_verify(dir, - (struct ext4_dir_entry *)bh->b_data)) { - ext4_warning(dir->i_sb, "Node failed checksum"); - brelse(bh); - *err = ERR_BAD_DX_DIR; - goto fail2; - } - set_buffer_verified(bh); - if (dx_get_limit(entries) != dx_node_limit (dir)) { ext4_warning(dir->i_sb, "dx entry: limit != node limit"); @@ -783,7 +844,7 @@ static int ext4_htree_next_block(struct inode *dir, __u32 hash, { struct dx_frame *p; struct buffer_head *bh; - int err, num_frames = 0; + int num_frames = 0; __u32 bhash; p = frame; @@ -822,26 +883,9 @@ static int ext4_htree_next_block(struct inode *dir, __u32 hash, * block so no check is necessary */ while (num_frames--) { - if (!(bh = ext4_bread(NULL, dir, dx_get_block(p->at), - 0, &err))) { - if (!err) { - ext4_error(dir->i_sb, - "Directory hole detected on inode %lu\n", - dir->i_ino); - return -EIO; - } - return err; /* Failure */ - } - - if (!buffer_verified(bh) && - !ext4_dx_csum_verify(dir, - (struct ext4_dir_entry *)bh->b_data)) { - ext4_warning(dir->i_sb, "Node failed checksum"); - brelse(bh); - return -EIO; - } - set_buffer_verified(bh); - + bh = ext4_read_dirblock(dir, dx_get_block(p->at), INDEX); + if (IS_ERR(bh)) + return PTR_ERR(bh); p++; brelse(p->bh); p->bh = bh; @@ -867,23 +911,9 @@ static int htree_dirblock_to_tree(struct file *dir_file, dxtrace(printk(KERN_INFO "In htree dirblock_to_tree: block %lu\n", (unsigned long)block)); - if (!(bh = ext4_bread(NULL, dir, block, 0, &err))) { - if (!err) { - err = -EIO; - ext4_error(dir->i_sb, - "Directory hole detected on inode %lu\n", - dir->i_ino); - } - return err; - } - - if (!buffer_verified(bh) && - !ext4_dirent_csum_verify(dir, - (struct ext4_dir_entry *)bh->b_data)) { - brelse(bh); - return -EIO; - } - set_buffer_verified(bh); + bh = ext4_read_dirblock(dir, block, DIRENT); + if (IS_ERR(bh)) + return PTR_ERR(bh); de = (struct ext4_dir_entry_2 *) bh->b_data; top = (struct ext4_dir_entry_2 *) ((char *) de + @@ -1337,26 +1367,11 @@ static struct buffer_head * ext4_dx_find_entry(struct inode *dir, const struct q return NULL; do { block = dx_get_block(frame->at); - if (!(bh = ext4_bread(NULL, dir, block, 0, err))) { - if (!(*err)) { - *err = -EIO; - ext4_error(dir->i_sb, - "Directory hole detected on inode %lu\n", - dir->i_ino); - } + bh = ext4_read_dirblock(dir, block, DIRENT); + if (IS_ERR(bh)) { + *err = PTR_ERR(bh); goto errout; } - - if (!buffer_verified(bh) && - !ext4_dirent_csum_verify(dir, - (struct ext4_dir_entry *)bh->b_data)) { - EXT4_ERROR_INODE(dir, "checksumming directory " - "block %lu", (unsigned long)block); - brelse(bh); - *err = -EIO; - goto errout; - } - set_buffer_verified(bh); retval = search_dirblock(bh, dir, d_name, block << EXT4_BLOCK_SIZE_BITS(sb), res_dir); @@ -1920,22 +1935,10 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, } blocks = dir->i_size >> sb->s_blocksize_bits; for (block = 0; block < blocks; block++) { - if (!(bh = ext4_bread(handle, dir, block, 0, &retval))) { - if (!retval) { - retval = -EIO; - ext4_error(inode->i_sb, - "Directory hole detected on inode %lu\n", - inode->i_ino); - } - return retval; - } - if (!buffer_verified(bh) && - !ext4_dirent_csum_verify(dir, - (struct ext4_dir_entry *)bh->b_data)) { - brelse(bh); - return -EIO; - } - set_buffer_verified(bh); + bh = ext4_read_dirblock(dir, block, DIRENT); + if (IS_ERR(bh)) + return PTR_ERR(bh); + retval = add_dirent_to_buf(handle, dentry, inode, NULL, bh); if (retval != -ENOSPC) { brelse(bh); @@ -1986,22 +1989,13 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry, return err; entries = frame->entries; at = frame->at; - - if (!(bh = ext4_bread(handle, dir, dx_get_block(frame->at), 0, &err))) { - if (!err) { - err = -EIO; - ext4_error(dir->i_sb, - "Directory hole detected on inode %lu\n", - dir->i_ino); - } + bh = ext4_read_dirblock(dir, dx_get_block(frame->at), DIRENT); + if (IS_ERR(bh)) { + err = PTR_ERR(bh); + bh = NULL; goto cleanup; } - if (!buffer_verified(bh) && - !ext4_dirent_csum_verify(dir, (struct ext4_dir_entry *)bh->b_data)) - goto journal_error; - set_buffer_verified(bh); - BUFFER_TRACE(bh, "get_write_access"); err = ext4_journal_get_write_access(handle, bh); if (err) @@ -2352,6 +2346,7 @@ static int ext4_init_new_dir(handle_t *handle, struct inode *dir, struct buffer_head *dir_block = NULL; struct ext4_dir_entry_2 *de; struct ext4_dir_entry_tail *t; + ext4_lblk_t block = 0; unsigned int blocksize = dir->i_sb->s_blocksize; int csum_size = 0; int err; @@ -2368,16 +2363,9 @@ static int ext4_init_new_dir(handle_t *handle, struct inode *dir, goto out; } - inode->i_size = EXT4_I(inode)->i_disksize = blocksize; - if (!(dir_block = ext4_bread(handle, inode, 0, 1, &err))) { - if (!err) { - err = -EIO; - ext4_error(inode->i_sb, - "Directory hole detected on inode %lu\n", - inode->i_ino); - } + inode->i_size = 0; + if (!(dir_block = ext4_append(handle, inode, &block, &err))) goto out; - } BUFFER_TRACE(dir_block, "get_write_access"); err = ext4_journal_get_write_access(handle, dir_block); if (err) @@ -2477,26 +2465,14 @@ static int empty_dir(struct inode *inode) } sb = inode->i_sb; - if (inode->i_size < EXT4_DIR_REC_LEN(1) + EXT4_DIR_REC_LEN(2) || - !(bh = ext4_bread(NULL, inode, 0, 0, &err))) { - if (err) - EXT4_ERROR_INODE(inode, - "error %d reading directory lblock 0", err); - else - ext4_warning(inode->i_sb, - "bad directory (dir #%lu) - no data block", - inode->i_ino); + if (inode->i_size < EXT4_DIR_REC_LEN(1) + EXT4_DIR_REC_LEN(2)) { + EXT4_ERROR_INODE(inode, "invalid size"); return 1; } - if (!buffer_verified(bh) && - !ext4_dirent_csum_verify(inode, - (struct ext4_dir_entry *)bh->b_data)) { - EXT4_ERROR_INODE(inode, "checksum error reading directory " - "lblock 0"); - brelse(bh); - return -EIO; - } - set_buffer_verified(bh); + bh = ext4_read_dirblock(inode, 0, EITHER); + if (IS_ERR(bh)) + return 1; + de = (struct ext4_dir_entry_2 *) bh->b_data; de1 = ext4_next_entry(de, sb->s_blocksize); if (le32_to_cpu(de->inode) != inode->i_ino || @@ -2519,29 +2495,9 @@ static int empty_dir(struct inode *inode) err = 0; brelse(bh); lblock = offset >> EXT4_BLOCK_SIZE_BITS(sb); - bh = ext4_bread(NULL, inode, lblock, 0, &err); - if (!bh) { - if (err) - EXT4_ERROR_INODE(inode, - "error %d reading directory " - "lblock %u", err, lblock); - else - ext4_warning(inode->i_sb, - "bad directory (dir #%lu) - no data block", - inode->i_ino); - - offset += sb->s_blocksize; - continue; - } - if (!buffer_verified(bh) && - !ext4_dirent_csum_verify(inode, - (struct ext4_dir_entry *)bh->b_data)) { - EXT4_ERROR_INODE(inode, "checksum error " - "reading directory lblock 0"); - brelse(bh); - return -EIO; - } - set_buffer_verified(bh); + bh = ext4_read_dirblock(inode, lblock, EITHER); + if (IS_ERR(bh)) + return 1; de = (struct ext4_dir_entry_2 *) bh->b_data; } if (ext4_check_dir_entry(inode, NULL, de, bh, @@ -3004,13 +2960,9 @@ static struct buffer_head *ext4_get_first_dir_block(handle_t *handle, struct buffer_head *bh; if (!ext4_has_inline_data(inode)) { - if (!(bh = ext4_bread(handle, inode, 0, 0, retval))) { - if (!*retval) { - *retval = -EIO; - ext4_error(inode->i_sb, - "Directory hole detected on inode %lu\n", - inode->i_ino); - } + bh = ext4_read_dirblock(inode, 0, EITHER); + if (IS_ERR(bh)) { + *retval = PTR_ERR(bh); return NULL; } *parent_de = ext4_next_entry( @@ -3089,11 +3041,6 @@ static int ext4_rename(struct inode *old_dir, struct dentry *old_dentry, &inlined); if (!dir_bh) goto end_rename; - if (!inlined && !buffer_verified(dir_bh) && - !ext4_dirent_csum_verify(old_inode, - (struct ext4_dir_entry *)dir_bh->b_data)) - goto end_rename; - set_buffer_verified(dir_bh); if (le32_to_cpu(parent_de->inode) != old_dir->i_ino) goto end_rename; retval = -EMLINK; -- cgit v1.1 From 0f70b40613ee14b0cadafeb461034cff81b4419a Mon Sep 17 00:00:00 2001 From: Theodore Ts'o Date: Fri, 15 Feb 2013 03:35:57 -0500 Subject: ext4: use ERR_PTR() abstraction for ext4_append() Use ERR_PTR()/IS_ERR() abstraction instead of passing in a separate pointer to an integer for the error code, as a code cleanup. Signed-off-by: "Theodore Ts'o" --- fs/ext4/namei.c | 65 +++++++++++++++++++++++++++------------------------------ 1 file changed, 31 insertions(+), 34 deletions(-) (limited to 'fs/ext4/namei.c') diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c index 0e28c74..f58c053 100644 --- a/fs/ext4/namei.c +++ b/fs/ext4/namei.c @@ -51,34 +51,28 @@ static struct buffer_head *ext4_append(handle_t *handle, struct inode *inode, - ext4_lblk_t *block, int *err) + ext4_lblk_t *block) { struct buffer_head *bh; + int err = 0; if (unlikely(EXT4_SB(inode->i_sb)->s_max_dir_size_kb && ((inode->i_size >> 10) >= - EXT4_SB(inode->i_sb)->s_max_dir_size_kb))) { - *err = -ENOSPC; - return NULL; - } + EXT4_SB(inode->i_sb)->s_max_dir_size_kb))) + return ERR_PTR(-ENOSPC); *block = inode->i_size >> inode->i_sb->s_blocksize_bits; - bh = ext4_bread(handle, inode, *block, 1, err); - if (bh) { - inode->i_size += inode->i_sb->s_blocksize; - EXT4_I(inode)->i_disksize = inode->i_size; - *err = ext4_journal_get_write_access(handle, bh); - if (*err) { - brelse(bh); - bh = NULL; - } - } - if (!bh && !(*err)) { - *err = -EIO; - ext4_error(inode->i_sb, - "Directory hole detected on inode %lu\n", - inode->i_ino); + bh = ext4_bread(handle, inode, *block, 1, &err); + if (!bh) + return ERR_PTR(err); + inode->i_size += inode->i_sb->s_blocksize; + EXT4_I(inode)->i_disksize = inode->i_size; + err = ext4_journal_get_write_access(handle, bh); + if (err) { + brelse(bh); + ext4_std_error(inode->i_sb, err); + return ERR_PTR(err); } return bh; } @@ -1555,11 +1549,12 @@ static struct ext4_dir_entry_2 *do_split(handle_t *handle, struct inode *dir, EXT4_FEATURE_RO_COMPAT_METADATA_CSUM)) csum_size = sizeof(struct ext4_dir_entry_tail); - bh2 = ext4_append (handle, dir, &newblock, &err); - if (!(bh2)) { + bh2 = ext4_append(handle, dir, &newblock); + if (IS_ERR(bh2)) { brelse(*bh); *bh = NULL; - goto errout; + *error = PTR_ERR(bh2); + return NULL; } BUFFER_TRACE(*bh, "get_write_access"); @@ -1640,7 +1635,6 @@ journal_error: brelse(bh2); *bh = NULL; ext4_std_error(dir->i_sb, err); -errout: *error = err; return NULL; } @@ -1815,10 +1809,10 @@ static int make_indexed_dir(handle_t *handle, struct dentry *dentry, len = ((char *) root) + (blocksize - csum_size) - (char *) de; /* Allocate new block for the 0th block's dirents */ - bh2 = ext4_append(handle, dir, &block, &retval); - if (!(bh2)) { + bh2 = ext4_append(handle, dir, &block); + if (IS_ERR(bh2)) { brelse(bh); - return retval; + return PTR_ERR(bh2); } ext4_set_inode_flag(dir, EXT4_INODE_INDEX); data1 = bh2->b_data; @@ -1950,9 +1944,9 @@ static int ext4_add_entry(handle_t *handle, struct dentry *dentry, return make_indexed_dir(handle, dentry, inode, bh); brelse(bh); } - bh = ext4_append(handle, dir, &block, &retval); - if (!bh) - return retval; + bh = ext4_append(handle, dir, &block); + if (IS_ERR(bh)) + return PTR_ERR(bh); de = (struct ext4_dir_entry_2 *) bh->b_data; de->inode = 0; de->rec_len = ext4_rec_len_to_disk(blocksize - csum_size, blocksize); @@ -2023,9 +2017,11 @@ static int ext4_dx_add_entry(handle_t *handle, struct dentry *dentry, err = -ENOSPC; goto cleanup; } - bh2 = ext4_append (handle, dir, &newblock, &err); - if (!(bh2)) + bh2 = ext4_append(handle, dir, &newblock); + if (IS_ERR(bh2)) { + err = PTR_ERR(bh2); goto cleanup; + } node2 = (struct dx_node *)(bh2->b_data); entries2 = node2->entries; memset(&node2->fake, 0, sizeof(struct fake_dirent)); @@ -2364,8 +2360,9 @@ static int ext4_init_new_dir(handle_t *handle, struct inode *dir, } inode->i_size = 0; - if (!(dir_block = ext4_append(handle, inode, &block, &err))) - goto out; + dir_block = ext4_append(handle, inode, &block); + if (IS_ERR(dir_block)) + return PTR_ERR(dir_block); BUFFER_TRACE(dir_block, "get_write_access"); err = ext4_journal_get_write_access(handle, dir_block); if (err) -- cgit v1.1