On Sat, Nov 14, 2009 at 07:30:59PM -0500, Theodore Ts'o wrote:
> When a directory gets unlinked, ext4_forget() is called on any buffer
> heads corresponding to its data blocks. Data blocks from directories
> must be treated as metadata, so that they are revoked by
> jbd2_journal_revoke, and not just forgotten via ext4_journal_forget().
>
> Thanks to Curt Wohlgemuth for pointing out potential problems in this
> area.
>
> Signed-off-by: "Theodore Ts'o" <tytso@xxxxxxx>
> ---
> fs/ext4/inode.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 13de1dd..639bb84 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -97,6 +97,10 @@ int ext4_forget(handle_t *handle, int is_metadata, struct inode *inode,
> bh, is_metadata, inode->i_mode,
> test_opt(inode->i_sb, DATA_FLAGS));
>
> + /* Directory blocks must be treated as metadata */
> + if (S_ISDIR(inode->i_mode))
> + is_metadata = 1;
> +
> /* Never use the revoke function if we are doing full data
> * journaling: there is no need to, and a V1 superblock won't
> * support it. Otherwise, only skip the revoke on un-journaled
I guess we need to make sure we call ext4_forget with correct is_metadata values. I
did the below patch. The xattr changes in the patch should be split as a separate one.
I am not sure why we do a get_bh there.
Another question i have is, do we actually supporting freeing directory blocks
when we delete directory entries ? I remember reading we don't have support for that.
So may be Curt is not seeing the ext4_forget being called because he is trying delete
of directory entries. I guess he will have to do a rmdir directory to see the
directory blocks freed.
If you think the changes are correct i will send proper patches with s-o-b
-aneesh
diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 715264b..74dcff8 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -2074,7 +2074,7 @@ static int ext4_remove_blocks(handle_t *handle, struct inode *inode,
ext_debug("free last %u blocks starting %llu\n", num, start);
for (i = 0; i < num; i++) {
bh = sb_find_get_block(inode->i_sb, start + i);
- ext4_forget(handle, 0, inode, bh, start + i);
+ ext4_forget(handle, metadata, inode, bh, start + i);
}
ext4_free_blocks(handle, inode, start, num, metadata);
} else if (from == le32_to_cpu(ex->ee_block)
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2c8caa5..87232a5 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -96,6 +96,13 @@ int ext4_forget(handle_t *handle, int is_metadata, struct inode *inode,
bh, is_metadata, inode->i_mode,
test_opt(inode->i_sb, DATA_FLAGS));
+ /*
+ * The caller should have set is_metadata properly. But
+ * being extra careful here
+ */
+ if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode))
+ is_metadata = 1;
+
/* Never use the revoke function if we are doing full data
* journaling: there is no need to, and a V1 superblock won't
* support it. Otherwise, only skip the revoke on un-journaled
diff --git a/fs/ext4/xattr.c b/fs/ext4/xattr.c
index fed5b01..3c93a9a 100644
--- a/fs/ext4/xattr.c
+++ b/fs/ext4/xattr.c
@@ -482,9 +482,8 @@ ext4_xattr_release_block(handle_t *handle, struct inode *inode,
ea_bdebug(bh, "refcount now=0; freeing");
if (ce)
mb_cache_entry_free(ce);
- ext4_free_blocks(handle, inode, bh->b_blocknr, 1, 1);
- get_bh(bh);
ext4_forget(handle, 1, inode, bh, bh->b_blocknr);
+ ext4_free_blocks(handle, inode, bh->b_blocknr, 1, 1);
} else {
le32_add_cpu(&BHDR(bh)->h_refcount, -1);
error = ext4_handle_dirty_metadata(handle, inode, bh);
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
