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 <sekha...@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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
