Tristan Ye
2010-Aug-27 02:46 UTC
[Ocfs2-devel] [PATCH 1/1] Ocfs2: Add missing ocfs2_journal_acces_*() for couple of funcs in dir.c
To correctly journal the metadata in ocfs2, it's known for us to call
ocfs2_journal_access_*() and ocfs2_journal_dirty()to mark buffer dirty,
they are expected to exist in a pair.
Whereas several funcs for dx-dirs manipulation were forgeting to call
appropriate ocfs2_journal_access*() to correctly journal the dirty
metadata, which may cause a BUG in jbd2, reporting a NULL pointer
gets ASSERTED in the journal buffer head.
Currently, we found three functions being hurt in dir.c, all serving
for dx-dirs:
- ocfs2_dx_dir_transfer_leaf()
- ocfs2_remove_block_from_free_list()
- ocfs2_recalc_free_list()
Signed-off-by: Tristan Ye <tristan.ye at oracle.com>
---
fs/ocfs2/dir.c | 109 +++++++++++++++++++++++++++++++++++++++++++++-----------
1 files changed, 88 insertions(+), 21 deletions(-)
diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c
index f04ebcf..fe89136 100644
--- a/fs/ocfs2/dir.c
+++ b/fs/ocfs2/dir.c
@@ -1556,41 +1556,67 @@ out:
return ret;
}
-static void ocfs2_remove_block_from_free_list(struct inode *dir,
- handle_t *handle,
- struct ocfs2_dir_lookup_result *lookup)
+static int ocfs2_remove_block_from_free_list(struct inode *dir,
+ handle_t *handle,
+ struct ocfs2_dir_lookup_result *lookup)
{
+ int status = 0;
struct ocfs2_dir_block_trailer *trailer, *prev;
struct ocfs2_dx_root_block *dx_root;
struct buffer_head *bh;
+ ocfs2_journal_access_func access = ocfs2_journal_access_dr;
trailer = ocfs2_trailer_from_bh(lookup->dl_leaf_bh, dir->i_sb);
if (ocfs2_free_list_at_root(lookup)) {
bh = lookup->dl_dx_root_bh;
+ status = access(handle, INODE_CACHE(dir), bh,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (status < 0) {
+ mlog_errno(status);
+ return status;
+ }
dx_root = (struct ocfs2_dx_root_block *)bh->b_data;
dx_root->dr_free_blk = trailer->db_free_next;
} else {
+ access = ocfs2_journal_access_db;
bh = lookup->dl_prev_leaf_bh;
+ status = access(handle, INODE_CACHE(dir), bh,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (status < 0) {
+ mlog_errno(status);
+ return status;
+ }
prev = ocfs2_trailer_from_bh(bh, dir->i_sb);
prev->db_free_next = trailer->db_free_next;
}
+ ocfs2_journal_dirty(handle, bh);
+
+ status = ocfs2_journal_access_db(handle, INODE_CACHE(dir),
+ lookup->dl_leaf_bh,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (status < 0) {
+ mlog_errno(status);
+ return status;
+ }
+
trailer->db_free_rec_len = cpu_to_le16(0);
trailer->db_free_next = cpu_to_le64(0);
- ocfs2_journal_dirty(handle, bh);
ocfs2_journal_dirty(handle, lookup->dl_leaf_bh);
+
+ return status;
}
/*
* This expects that a journal write has been reserved on
* lookup->dl_prev_leaf_bh or lookup->dl_dx_root_bh
*/
-static void ocfs2_recalc_free_list(struct inode *dir, handle_t *handle,
- struct ocfs2_dir_lookup_result *lookup)
+static int ocfs2_recalc_free_list(struct inode *dir, handle_t *handle,
+ struct ocfs2_dir_lookup_result *lookup)
{
- int max_rec_len;
+ int max_rec_len, status = 0;
struct ocfs2_dir_block_trailer *trailer;
/* Walk dl_leaf_bh to figure out what the new free rec_len is. */
@@ -1601,12 +1627,24 @@ static void ocfs2_recalc_free_list(struct inode *dir,
handle_t *handle,
* from the free list. In this case, we just want to update
* the rec len accounting.
*/
+ status = ocfs2_journal_access_db(handle, INODE_CACHE(dir),
+ lookup->dl_leaf_bh,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (status < 0) {
+ mlog_errno(status);
+ return status;
+ }
+
trailer = ocfs2_trailer_from_bh(lookup->dl_leaf_bh, dir->i_sb);
trailer->db_free_rec_len = cpu_to_le16(max_rec_len);
ocfs2_journal_dirty(handle, lookup->dl_leaf_bh);
} else {
- ocfs2_remove_block_from_free_list(dir, handle, lookup);
+ status = ocfs2_remove_block_from_free_list(dir, handle, lookup);
+ if (status < 0)
+ mlog_errno(status);
}
+
+ return status;
}
/* we don't always have a dentry for what we want to add, so people
@@ -1748,8 +1786,14 @@ int __ocfs2_add_entry(handle_t *handle,
de->name_len = namelen;
memcpy(de->name, name, namelen);
- if (ocfs2_dir_indexed(dir))
- ocfs2_recalc_free_list(dir, handle, lookup);
+ if (ocfs2_dir_indexed(dir)) {
+ retval = ocfs2_recalc_free_list(dir, handle,
+ lookup);
+ if (retval < 0) {
+ mlog_errno(retval);
+ goto bail;
+ }
+ }
dir->i_version++;
ocfs2_journal_dirty(handle, insert_bh);
@@ -3736,14 +3780,14 @@ static int ocfs2_dx_dir_find_leaf_split(struct
ocfs2_dx_leaf *dx_leaf,
* of minor_hash, we can optimize - an item at block offset X within
* the original cluster, will be at offset X within the new cluster.
*/
-static void ocfs2_dx_dir_transfer_leaf(struct inode *dir, u32 split_hash,
- handle_t *handle,
- struct ocfs2_dx_leaf *tmp_dx_leaf,
- struct buffer_head **orig_dx_leaves,
- struct buffer_head **new_dx_leaves,
- int num_dx_leaves)
+static int ocfs2_dx_dir_transfer_leaf(struct inode *dir, u32 split_hash,
+ handle_t *handle,
+ struct ocfs2_dx_leaf *tmp_dx_leaf,
+ struct buffer_head **orig_dx_leaves,
+ struct buffer_head **new_dx_leaves,
+ int num_dx_leaves)
{
- int i, j, num_used;
+ int i, j, num_used, status = 0;
u32 major_hash;
struct ocfs2_dx_leaf *orig_dx_leaf, *new_dx_leaf;
struct ocfs2_dx_entry_list *orig_list, *new_list, *tmp_list;
@@ -3763,6 +3807,14 @@ static void ocfs2_dx_dir_transfer_leaf(struct inode *dir,
u32 split_hash,
tmp_list->de_num_used = cpu_to_le16(0);
memset(&tmp_list->de_entries, 0, sizeof(*dx_entry)*num_used);
+ status = ocfs2_journal_access_dl(handle, INODE_CACHE(dir),
+ new_dx_leaves[i],
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (status < 0) {
+ mlog_errno(status);
+ return status;
+ }
+
for (j = 0; j < num_used; j++) {
dx_entry = &orig_list->de_entries[j];
major_hash = le32_to_cpu(dx_entry->dx_major_hash);
@@ -3773,11 +3825,23 @@ static void ocfs2_dx_dir_transfer_leaf(struct inode
*dir, u32 split_hash,
ocfs2_dx_dir_leaf_insert_tail(tmp_dx_leaf,
dx_entry);
}
+
+ ocfs2_journal_dirty(handle, new_dx_leaves[i]);
+
+ status = ocfs2_journal_access_dl(handle, INODE_CACHE(dir),
+ orig_dx_leaves[i],
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (status < 0) {
+ mlog_errno(status);
+ return status;
+ }
+
memcpy(orig_dx_leaf, tmp_dx_leaf, dir->i_sb->s_blocksize);
ocfs2_journal_dirty(handle, orig_dx_leaves[i]);
- ocfs2_journal_dirty(handle, new_dx_leaves[i]);
}
+
+ return status;
}
static int ocfs2_dx_dir_rebalance_credits(struct ocfs2_super *osb,
@@ -3950,8 +4014,11 @@ static int ocfs2_dx_dir_rebalance(struct ocfs2_super
*osb, struct inode *dir,
goto out_commit;
}
- ocfs2_dx_dir_transfer_leaf(dir, split_hash, handle, tmp_dx_leaf,
- orig_dx_leaves, new_dx_leaves, num_dx_leaves);
+ ret = ocfs2_dx_dir_transfer_leaf(dir, split_hash, handle, tmp_dx_leaf,
+ orig_dx_leaves, new_dx_leaves,
+ num_dx_leaves);
+ if (ret)
+ mlog_errno(ret);
out_commit:
if (ret < 0 && did_quota)
--
1.5.5
tristan
2010-Sep-01 01:25 UTC
[Ocfs2-devel] [PATCH 1/1] Ocfs2: Add missing ocfs2_journal_acces_*() for couple of funcs in dir.c
Hi Mark, Could you please take a glance at this patch:-) Tristan. Tristan Ye wrote:> To correctly journal the metadata in ocfs2, it's known for us to call > ocfs2_journal_access_*() and ocfs2_journal_dirty()to mark buffer dirty, > they are expected to exist in a pair. > > Whereas several funcs for dx-dirs manipulation were forgeting to call > appropriate ocfs2_journal_access*() to correctly journal the dirty > metadata, which may cause a BUG in jbd2, reporting a NULL pointer > gets ASSERTED in the journal buffer head. > > Currently, we found three functions being hurt in dir.c, all serving > for dx-dirs: > > - ocfs2_dx_dir_transfer_leaf() > - ocfs2_remove_block_from_free_list() > - ocfs2_recalc_free_list() > > Signed-off-by: Tristan Ye <tristan.ye at oracle.com> > --- > fs/ocfs2/dir.c | 109 +++++++++++++++++++++++++++++++++++++++++++++----------- > 1 files changed, 88 insertions(+), 21 deletions(-) > > diff --git a/fs/ocfs2/dir.c b/fs/ocfs2/dir.c > index f04ebcf..fe89136 100644 > --- a/fs/ocfs2/dir.c > +++ b/fs/ocfs2/dir.c > @@ -1556,41 +1556,67 @@ out: > return ret; > } > > -static void ocfs2_remove_block_from_free_list(struct inode *dir, > - handle_t *handle, > - struct ocfs2_dir_lookup_result *lookup) > +static int ocfs2_remove_block_from_free_list(struct inode *dir, > + handle_t *handle, > + struct ocfs2_dir_lookup_result *lookup) > { > + int status = 0; > struct ocfs2_dir_block_trailer *trailer, *prev; > struct ocfs2_dx_root_block *dx_root; > struct buffer_head *bh; > + ocfs2_journal_access_func access = ocfs2_journal_access_dr; > > trailer = ocfs2_trailer_from_bh(lookup->dl_leaf_bh, dir->i_sb); > > if (ocfs2_free_list_at_root(lookup)) { > bh = lookup->dl_dx_root_bh; > + status = access(handle, INODE_CACHE(dir), bh, > + OCFS2_JOURNAL_ACCESS_WRITE); > + if (status < 0) { > + mlog_errno(status); > + return status; > + } > dx_root = (struct ocfs2_dx_root_block *)bh->b_data; > dx_root->dr_free_blk = trailer->db_free_next; > } else { > + access = ocfs2_journal_access_db; > bh = lookup->dl_prev_leaf_bh; > + status = access(handle, INODE_CACHE(dir), bh, > + OCFS2_JOURNAL_ACCESS_WRITE); > + if (status < 0) { > + mlog_errno(status); > + return status; > + } > prev = ocfs2_trailer_from_bh(bh, dir->i_sb); > prev->db_free_next = trailer->db_free_next; > } > > + ocfs2_journal_dirty(handle, bh); > + > + status = ocfs2_journal_access_db(handle, INODE_CACHE(dir), > + lookup->dl_leaf_bh, > + OCFS2_JOURNAL_ACCESS_WRITE); > + if (status < 0) { > + mlog_errno(status); > + return status; > + } > + > trailer->db_free_rec_len = cpu_to_le16(0); > trailer->db_free_next = cpu_to_le64(0); > > - ocfs2_journal_dirty(handle, bh); > ocfs2_journal_dirty(handle, lookup->dl_leaf_bh); > + > + return status; > } > > /* > * This expects that a journal write has been reserved on > * lookup->dl_prev_leaf_bh or lookup->dl_dx_root_bh > */ > -static void ocfs2_recalc_free_list(struct inode *dir, handle_t *handle, > - struct ocfs2_dir_lookup_result *lookup) > +static int ocfs2_recalc_free_list(struct inode *dir, handle_t *handle, > + struct ocfs2_dir_lookup_result *lookup) > { > - int max_rec_len; > + int max_rec_len, status = 0; > struct ocfs2_dir_block_trailer *trailer; > > /* Walk dl_leaf_bh to figure out what the new free rec_len is. */ > @@ -1601,12 +1627,24 @@ static void ocfs2_recalc_free_list(struct inode *dir, handle_t *handle, > * from the free list. In this case, we just want to update > * the rec len accounting. > */ > + status = ocfs2_journal_access_db(handle, INODE_CACHE(dir), > + lookup->dl_leaf_bh, > + OCFS2_JOURNAL_ACCESS_WRITE); > + if (status < 0) { > + mlog_errno(status); > + return status; > + } > + > trailer = ocfs2_trailer_from_bh(lookup->dl_leaf_bh, dir->i_sb); > trailer->db_free_rec_len = cpu_to_le16(max_rec_len); > ocfs2_journal_dirty(handle, lookup->dl_leaf_bh); > } else { > - ocfs2_remove_block_from_free_list(dir, handle, lookup); > + status = ocfs2_remove_block_from_free_list(dir, handle, lookup); > + if (status < 0) > + mlog_errno(status); > } > + > + return status; > } > > /* we don't always have a dentry for what we want to add, so people > @@ -1748,8 +1786,14 @@ int __ocfs2_add_entry(handle_t *handle, > de->name_len = namelen; > memcpy(de->name, name, namelen); > > - if (ocfs2_dir_indexed(dir)) > - ocfs2_recalc_free_list(dir, handle, lookup); > + if (ocfs2_dir_indexed(dir)) { > + retval = ocfs2_recalc_free_list(dir, handle, > + lookup); > + if (retval < 0) { > + mlog_errno(retval); > + goto bail; > + } > + } > > dir->i_version++; > ocfs2_journal_dirty(handle, insert_bh); > @@ -3736,14 +3780,14 @@ static int ocfs2_dx_dir_find_leaf_split(struct ocfs2_dx_leaf *dx_leaf, > * of minor_hash, we can optimize - an item at block offset X within > * the original cluster, will be at offset X within the new cluster. > */ > -static void ocfs2_dx_dir_transfer_leaf(struct inode *dir, u32 split_hash, > - handle_t *handle, > - struct ocfs2_dx_leaf *tmp_dx_leaf, > - struct buffer_head **orig_dx_leaves, > - struct buffer_head **new_dx_leaves, > - int num_dx_leaves) > +static int ocfs2_dx_dir_transfer_leaf(struct inode *dir, u32 split_hash, > + handle_t *handle, > + struct ocfs2_dx_leaf *tmp_dx_leaf, > + struct buffer_head **orig_dx_leaves, > + struct buffer_head **new_dx_leaves, > + int num_dx_leaves) > { > - int i, j, num_used; > + int i, j, num_used, status = 0; > u32 major_hash; > struct ocfs2_dx_leaf *orig_dx_leaf, *new_dx_leaf; > struct ocfs2_dx_entry_list *orig_list, *new_list, *tmp_list; > @@ -3763,6 +3807,14 @@ static void ocfs2_dx_dir_transfer_leaf(struct inode *dir, u32 split_hash, > tmp_list->de_num_used = cpu_to_le16(0); > memset(&tmp_list->de_entries, 0, sizeof(*dx_entry)*num_used); > > + status = ocfs2_journal_access_dl(handle, INODE_CACHE(dir), > + new_dx_leaves[i], > + OCFS2_JOURNAL_ACCESS_WRITE); > + if (status < 0) { > + mlog_errno(status); > + return status; > + } > + > for (j = 0; j < num_used; j++) { > dx_entry = &orig_list->de_entries[j]; > major_hash = le32_to_cpu(dx_entry->dx_major_hash); > @@ -3773,11 +3825,23 @@ static void ocfs2_dx_dir_transfer_leaf(struct inode *dir, u32 split_hash, > ocfs2_dx_dir_leaf_insert_tail(tmp_dx_leaf, > dx_entry); > } > + > + ocfs2_journal_dirty(handle, new_dx_leaves[i]); > + > + status = ocfs2_journal_access_dl(handle, INODE_CACHE(dir), > + orig_dx_leaves[i], > + OCFS2_JOURNAL_ACCESS_WRITE); > + if (status < 0) { > + mlog_errno(status); > + return status; > + } > + > memcpy(orig_dx_leaf, tmp_dx_leaf, dir->i_sb->s_blocksize); > > ocfs2_journal_dirty(handle, orig_dx_leaves[i]); > - ocfs2_journal_dirty(handle, new_dx_leaves[i]); > } > + > + return status; > } > > static int ocfs2_dx_dir_rebalance_credits(struct ocfs2_super *osb, > @@ -3950,8 +4014,11 @@ static int ocfs2_dx_dir_rebalance(struct ocfs2_super *osb, struct inode *dir, > goto out_commit; > } > > - ocfs2_dx_dir_transfer_leaf(dir, split_hash, handle, tmp_dx_leaf, > - orig_dx_leaves, new_dx_leaves, num_dx_leaves); > + ret = ocfs2_dx_dir_transfer_leaf(dir, split_hash, handle, tmp_dx_leaf, > + orig_dx_leaves, new_dx_leaves, > + num_dx_leaves); > + if (ret) > + mlog_errno(ret); > > out_commit: > if (ret < 0 && did_quota) >
Mark Fasheh
2010-Sep-07 21:18 UTC
[Ocfs2-devel] [PATCH 1/1] Ocfs2: Add missing ocfs2_journal_acces_*() for couple of funcs in dir.c
On Fri, Aug 27, 2010 at 10:46:09AM +0800, Tristan Ye wrote:> To correctly journal the metadata in ocfs2, it's known for us to call > ocfs2_journal_access_*() and ocfs2_journal_dirty()to mark buffer dirty, > they are expected to exist in a pair. > > Whereas several funcs for dx-dirs manipulation were forgeting to call > appropriate ocfs2_journal_access*() to correctly journal the dirty > metadata, which may cause a BUG in jbd2, reporting a NULL pointer > gets ASSERTED in the journal buffer head. > > Currently, we found three functions being hurt in dir.c, all serving > for dx-dirs: > > - ocfs2_dx_dir_transfer_leaf()NAK, this doesn't look to have a problem. Both clusters (new and old) are journal_accessed previously. Look at ocfs2_dx_dir_rebalance and ocfs2_dx_dir_format_cluster.> - ocfs2_remove_block_from_free_list() > - ocfs2_recalc_free_list()Can you show me the stack traces for these please? It kinda looks like you just added journal_access calls randomly to make sure we cover everything. Not only does that confuse the code (look at how many functions turned from returning void to int) but it's redunant in most of the cases. --Mark -- Mark Fasheh