On Fri, Jul 11, 2008 at 04:27:21PM -0700, Sunil Mushran wrote:
> + if (replayed)
> + le32_add_cpu(&(fe->id1.journal1.ij_recovery_generation), 1);
You're doing this a lot, and it's a bunch to read. Can we have:
static void ocfs2_bump_recovery_generation(struct ocfs2_dinode *di)
{
le32_add_cpu(&(di->id1.journal1.ij_recovery_generation), 1);
}
static u32 ocfs2_get_recovery_generation(struct ocfs2_dinode *di)
{
return le32_to_cpu(di->id1.journal1.ij_recovery_generation);
}
I just think it'll read better.
> +static int ocfs2_read_journal_inode(struct ocfs2_super *osb,
> + int slot_num,
> + struct buffer_head **bh,
> + struct inode **ret_inode)
Perhaps you want to make ret_inode optional? It seems to be
there are a couple paths that are just iput()ing it. So, you'd do:
> +{
> + int status = -EACCES;
> + struct inode *inode = NULL;
> +
> + BUG_ON(slot_num >= osb->max_slots);
> +
- + *ret_inode = NULL;
- +
> + inode = ocfs2_get_system_file_inode(osb, JOURNAL_SYSTEM_INODE,
> + slot_num);
> + if (!inode || is_bad_inode(inode)) {
> + mlog_errno(status);
> + goto bail;
> + }
> + SET_INODE_JOURNAL(inode);
> +
> + status = ocfs2_read_block(osb, OCFS2_I(inode)->ip_blkno, bh, 0, inode);
> + if (status < 0) {
> + mlog_errno(status);
> + goto bail;
> + }
> +
> + status = 0;
> +
> +bail:
> + if (inode) {
- + if (status)
+ + if (status || !ret_inode)
> + iput(inode);
> + else
> + *ret_inode = inode;
> + }
> + return status;
> +}
<snip>
> flags = le32_to_cpu(fe->id1.journal1.ij_flags);
> + slot_reco_gen = le32_to_cpu(fe->id1.journal1.ij_recovery_generation);
Wouldn't this be so much nicer?
+ slot_reco_gen = ocfs2_get_recovery_generation(fe);
> @@ -1190,14 +1263,35 @@ int ocfs2_mark_dead_nodes(struct ocfs2_super *osb)
> {
> int status, i, node_num;
> struct ocfs2_slot_info *si = osb->slot_info;
> + struct buffer_head *bh = NULL;
> + struct ocfs2_dinode *fe;
If you are adding new dinodes, can we call them 'di' instead of
'fe'?
> spin_lock(&si->si_lock);
> for(i = 0; i < si->si_num_slots; i++) {
> + /* Read journal inode to get the recovery generation */
> + status = ocfs2_read_journal_inode(osb, i, &bh, &inode);
> + if (status) {
> + mlog_errno(status);
> + goto bail;
> + }
> + fe = (struct ocfs2_dinode *)bh->b_data;
> + osb->slot_recovery_generation[i] =
> + le32_to_cpu(fe->id1.journal1.ij_recovery_generation);
> + brelse(bh);
> + iput(inode);
Here's a place you'd want to pass NULL instead of &inode to
ocfs2_read_journal_inode();
Joel
--
Life's Little Instruction Book #197
"Don't forget, a person's greatest emotional need is to
feel appreciated."
Joel Becker
Principal Software Developer
Oracle
E-mail: [EMAIL PROTECTED]
Phone: (650) 506-8127
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
