Re: [PATCH v2] ext4: fix hole punch failure when depth is greater than 0
From: Ashish Sangwan <hidden>
Date: 2012-07-05 09:52:07
Also in:
lkml
On Wed, Jul 4, 2012 at 11:03 PM, Lukáš Czerner [off-list ref] wrote:
So I've finally has some time to look at the patch and reproduce the problem. Thanks for noticing the problem, the patch seems good, though I have one question. Is the p_block setting really necessary ? I do not think so, but I might be missing something. Here is updated patch I've tested, bellow.
AFAICS, p_block setting is necessary. As mentioned in the patch description,
whether to continue removing extents or not is decided by the return value
of function ext4_ext_more_to_rm() which checks 2 conditions:
a) if there are no more indexes to process.
b) if the number of entries are decreased in the header of "depth -1".
The p_block setting is important for condition b).
In function ext4_ext_more_to_rm, there is this second check:
if (le16_to_cpu(path->p_hdr->eh_entries) == path->p_block)
return 0;
If the value of p_block would not be correct, the above mentioned
condition could become
true while there are still blocks left to be removed.Note: there are some indent problems in your patch, like for example this: + path[k].p_block = + le16_to_cpu(path[k].p_hdr->eh_entries)+1;
Before submitting the patch, I run checkpatch.pl with --strict option. It did'nt show any error or warning. Should I re-submit the patch with an extra tab before the second line? The call is yours.
Anyway, what do you think about the modification ?
Also there is 1 modification missing from your patch.
ext4_ext_drop_refs(path);
kfree(path);
+ path = NULL;
if (err == -EAGAIN)
goto again;
If path is not set to NULL, it will crash in xfstest #13. Ted has
already reported it.
Thanks,
Ashish
quoted hunk ↗ jump to hunk
Thanks! -Lukasdiff --git a/fs/ext4/extents.c b/fs/ext4/extents.c index b3300eb..94bc1bd 100644 --- a/fs/ext4/extents.c +++ b/fs/ext4/extents.c@@ -2570,7 +2570,7 @@ static int ext4_ext_remove_space(struct inode *inode, ext4_lblk_t start, { struct super_block *sb = inode->i_sb; int depth = ext_depth(inode); - struct ext4_ext_path *path; + struct ext4_ext_path *path = NULL; ext4_fsblk_t partial_cluster = 0; handle_t *handle; int i, err;@@ -2606,8 +2606,12 @@ again: } depth = ext_depth(inode); ex = path[depth].p_ext; - if (!ex) + if (!ex) { + ext4_ext_drop_refs(path); + kfree(path); + path = NULL; goto cont; + } ee_block = le32_to_cpu(ex->ee_block);@@ -2637,8 +2641,6 @@ again: if (err < 0) goto out; } - ext4_ext_drop_refs(path); - kfree(path); } cont:@@ -2646,20 +2648,26 @@ cont: * We start scanning from right side, freeing all the blocks * after i_size and walking into the tree depth-wise. */ - depth = ext_depth(inode); - path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), GFP_NOFS); - if (path == NULL) { - ext4_journal_stop(handle); - return -ENOMEM; - } - path[0].p_depth = depth; - path[0].p_hdr = ext_inode_hdr(inode); + if (path) + i = depth; + else { + depth = ext_depth(inode); + path = kzalloc(sizeof(struct ext4_ext_path) * (depth + 1), + GFP_NOFS); + if (path == NULL) { + ext4_journal_stop(handle); + return -ENOMEM; + } + path[0].p_depth = depth; + path[0].p_hdr = ext_inode_hdr(inode); - if (ext4_ext_check(inode, path[0].p_hdr, depth)) { - err = -EIO; - goto out; + if (ext4_ext_check(inode, path[0].p_hdr, depth)) { + err = -EIO; + goto out; + } + i = 0; } - i = err = 0; + err = 0; while (i >= 0 && err == 0) { if (i == depth) {On Mon, 2 Jul 2012, Ashish Sangwan wrote:
-- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html