Hi Joel,
sorry, I missed this mail last time.
Joel Becker wrote:
> On Thu, Apr 30, 2009 at 06:58:19AM +0800, Tao Ma wrote:
>> +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"?
yeah, I will add the mlog.
>
>> + 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).
um, actually the b-tree code doesn't care it, so it means that we can
add a flag while clearing a flag in the same transaction here. So I will
change it to allow them to coexist. Maybe we need it in the future. ;)
Thanks.
Regards,
Tao
_______________________________________________
Ocfs2-devel mailing list
[email protected]
http://oss.oracle.com/mailman/listinfo/ocfs2-devel
