Re: [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case

[Chandra Seetharaman <sekharan@us.ibm.com>]

On Fri, 2013-10-04 at 14:38 -0700, Zach Brown wrote:

Thanks for the review Zach.

> On Fri, Oct 04, 2013 at 02:55:29PM -0500, Chandra Seetharaman wrote:
> > alloc_extent_buffer() uses radix_tree_lookup() when radix_tree_insert() fails
> > with EEXIST. That part of the code is very similar to the code in
> > find_extent_buffer(). This patch replaces radix_tree_lookup() and surrounding
> > code in alloc_extent_buffer() with find_extent_buffer().
> > 
> > While at it, this patch also changes the other usage of radix_tree_lookup() in
> > alloc_extent_buffer() with find_extent_buffer() to reduce redundancy.
> > 
> > Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com>
> 
> 
> > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> > index c09a40d..50345fb 100644
> > --- a/fs/btrfs/extent_io.c
> > +++ b/fs/btrfs/extent_io.c
> > @@ -4451,6 +4451,23 @@ static void mark_extent_buffer_accessed(struct extent_buffer *eb)
> >  	}
> >  }
> >  
> > +struct extent_buffer *find_extent_buffer(struct extent_io_tree *tree,
> > +					 u64 start, unsigned long len)
> 
> len isn't used.

Thought about removing the unused argument. But didn't know all the
history behind why it was there in the first place. So, didn't bother
it.

You think I should remove it.
 
> 
> > @@ -4530,24 +4543,17 @@ again:
> >  
> >  	spin_lock(&tree->buffer_lock);
> >  	ret = radix_tree_insert(&tree->buffer, start >> PAGE_CACHE_SHIFT, eb);
> > +	spin_unlock(&tree->buffer_lock);
> > +	radix_tree_preload_end();
> >  	if (ret == -EEXIST) {
> > -		exists = radix_tree_lookup(&tree->buffer,
> > -						start >> PAGE_CACHE_SHIFT);
> > -		if (!atomic_inc_not_zero(&exists->refs)) {
> > -			spin_unlock(&tree->buffer_lock);
> > -			radix_tree_preload_end();
> > -			exists = NULL;
> > +		exists = find_extent_buffer(tree, start, len);
> 
> Is it safe to do the lookup under rcu instead of under the spinlock?

Yes. it is fine.

eb->refs is what is used to make sure the buffer remains in the tree. eb
will be removed from the tree only if eb->refs is zero.
find_extent_buffer() will return NULL if refs is already zero. 

Besides, prior to this patch, there are other two instances of
radix_tree_lookup() (one in find_extent_buffer() and another in
alloc_extent_buffer()) which uses only rcu protection. 

> The commit message should mention the difference.

How does this commit message sound:
---------------
alloc_extent_buffer() uses radix_tree_lookup() when radix_tree_insert() fails
with EEXIST. That part of the code is very similar to the code in
find_extent_buffer(). This patch replaces radix_tree_lookup() and
surrounding code in alloc_extent_buffer() with find_extent_buffer().

Note that radix_tree_lookup() does not need to be protected by
tree->buffer_lock. It is protected by eb->refs.

While at it, this patch also changes the other usage of
radix_tree_lookup() in alloc_extent_buffer() with find_extent_buffer()
to reduce redundancy.
---------------

> 
> - z
>