On Tue, Mar 22, 2016 at 11:37:54AM +0100, Jan Kara wrote:
> On Tue 22-03-16 05:27:08, Matthew Wilcox wrote:
> > On Tue, Mar 22, 2016 at 10:12:32AM +0100, Jan Kara wrote:
> > > if (unlikely(!page)) // False since
> > > // RADIX_TREE_INDIRECT_PTR is set
> > > if (radix_tree_exception(page)) // False - no exeptional bit
> >
> > Oops, you got confused:
> >
> > static inline int radix_tree_exception(void *arg)
> > {
> > return unlikely((unsigned long)arg &
> > (RADIX_TREE_INDIRECT_PTR | RADIX_TREE_EXCEPTIONAL_ENTRY));
> > }
>
> Ah, I've confused radix_tree_exception() and
> radix_tree_exceptional_entry(). OK, so your code works AFAICT. But using
> RADIX_TREE_RETRY still doesn't make things clearer to me - you still need
> to check for INDIRECT bit in the retry logic to catch the
> radix_tree_extend() race as well...
>
> As a side note I think we should do away with radix_tree_exception() - it
> isn't very useful (doesn't simplify any of its callers) and only creates
> possibility for confusion.
Perhaps it would be clearer if we explicitly enumerated the four radix tree
entry types?
#define RADIX_TREE_TYPE_MASK 3
#define RADIX_TREE_TYPE_DATA 0
#define RADIX_TREE_TYPE_INDIRECT 1
#define RADIX_TREE_TYPE_EXCEPTIONAL 2
#define RADIX_TREE_TYPE_LOCKED_EXC 3
This would make radix_tree_exception (which we could rename so it doesn't
get confused with "exceptional" entries):
static inline int radix_tree_non_data(void *arg)
{
return unlikely((unsigned long)arg & RADIX_TREE_TYPE_MASK);
}
Etc? I guess we'd have to code it up and see if the result was simpler, but
it seems like it might be.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
