On Thu, Apr 30, 2009 at 06:58:19AM +0800, Tao Ma wrote:
> /*
> - * Mark part or all of the extent record at split_index in the leaf
> - * pointed to by path as written. This removes the unwritten
> - * extent flag.
> - *
> + * Split part or all of the extent record at split_index in the leaf
> + * pointed to by path. Merge with the contiguous extent record if needed.
> +
> * Care is taken to handle contiguousness so as to not grow the tree.
> *
> * meta_ac is not strictly necessary - we only truly need it if growth
There's a spurious blank line added here. I think you wanted it
to actually be ' *'.
> +static int ocfs2_change_extent_flag(handle_t *handle,
> + struct ocfs2_extent_tree *et,
> + u32 cpos, u32 len, u32 phys,
> + struct ocfs2_alloc_context *meta_ac,
> + struct ocfs2_cached_dealloc_ctxt *dealloc,
> + int new_flags, int clear_flags)
This function wants a comment about what it is for.
> + rec = &el->l_recs[index];
> + if ((new_flags && (rec->e_flags & new_flags)) ||
> + (clear_flags && !(rec->e_flags & clear_flags))) {
> + ret = -EIO;
> + mlog_errno(ret);
> + goto out;
> + }
Do we want to log some details as to where the -EIO came from?
Something like "Owner %llu tried to set flags on an extent that already
had them" and "tried to clear flags on an extent that didn't have them"?
> + if (new_flags)
> + split_rec.e_flags |= new_flags;
> + else
> + split_rec.e_flags &= ~clear_flags;
This code here assumes that the caller sets new_flags or
clear_flags but not both. Do we want that? Either we allow both, and
we honor both, or we should BUG_ON(new_flags && clear_flags).
Joel
--
"In a crisis, don't hide behind anything or anybody. They're going
to find you anyway."
- Paul "Bear" Bryant
Joel Becker
Principal Software Developer
Oracle
E-mail: [email protected]
Phone: (650) 506-8127
_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
