thr3ads.net - Btrfs devel - [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case [Oct 2013]

If this information is useful, please help other people find it:
Share via:

Chandra Seetharaman

2013-Oct-04 19:55 UTC

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

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)
+{
+	struct extent_buffer *eb;
+
+	rcu_read_lock();
+	eb = radix_tree_lookup(&tree->buffer, start >> PAGE_CACHE_SHIFT);
+	if (eb && atomic_inc_not_zero(&eb->refs)) {
+		rcu_read_unlock();
+		mark_extent_buffer_accessed(eb);
+		return eb;
+	}
+	rcu_read_unlock();
+
+	return NULL;
+}
+
 struct extent_buffer *alloc_extent_buffer(struct extent_io_tree *tree,
 					  u64 start, unsigned long len)
 {
@@ -4464,14 +4481,10 @@ struct extent_buffer *alloc_extent_buffer(struct
extent_io_tree *tree,
 	int uptodate = 1;
 	int ret;
 
-	rcu_read_lock();
-	eb = radix_tree_lookup(&tree->buffer, start >> PAGE_CACHE_SHIFT);
-	if (eb && atomic_inc_not_zero(&eb->refs)) {
-		rcu_read_unlock();
-		mark_extent_buffer_accessed(eb);
+
+	eb = find_extent_buffer(tree, start, len);
+	if (eb)
 		return eb;
-	}
-	rcu_read_unlock();
 
 	eb = __alloc_extent_buffer(tree, start, len, GFP_NOFS);
 	if (!eb)
@@ -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);
+		if (exists)
+			goto free_eb;
+		else
 			goto again;
-		}
-		spin_unlock(&tree->buffer_lock);
-		radix_tree_preload_end();
-		mark_extent_buffer_accessed(exists);
-		goto free_eb;
 	}
 	/* add one reference for the tree */
 	check_buffer_tree_ref(eb);
-	spin_unlock(&tree->buffer_lock);
-	radix_tree_preload_end();
 
 	/*
 	 * there is a race where release page may have
@@ -4578,23 +4584,6 @@ free_eb:
 	return exists;
 }
 
-struct extent_buffer *find_extent_buffer(struct extent_io_tree *tree,
-					 u64 start, unsigned long len)
-{
-	struct extent_buffer *eb;
-
-	rcu_read_lock();
-	eb = radix_tree_lookup(&tree->buffer, start >> PAGE_CACHE_SHIFT);
-	if (eb && atomic_inc_not_zero(&eb->refs)) {
-		rcu_read_unlock();
-		mark_extent_buffer_accessed(eb);
-		return eb;
-	}
-	rcu_read_unlock();
-
-	return NULL;
-}
-
 static inline void btrfs_release_extent_buffer_rcu(struct rcu_head *head)
 {
 	struct extent_buffer *eb 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zach Brown

2013-Oct-04 21:38 UTC

head link

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

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.
> @@ -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?
The commit message should mention the difference.

- z
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chandra Seetharaman

2013-Oct-04 22:00 UTC

head link

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

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
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Zach Brown

2013-Oct-04 22:09 UTC

head link

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

> > > +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.
Yeah, I would remove it.  Leave the tiny job of adding it to whoever
needs it in the future.  Until then we don''t need the noise.
> How does this commit message sound:
Yup, seems fine to me.

Reviewed-by: Zach Brown <zab@redhat.com>

- z
--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Chandra Seetharaman

2013-Oct-07 15:45 UTC

head link

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

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 
  - changes the other usage of radix_tree_lookup() in alloc_extent_buffer()
    with find_extent_buffer() to reduce redundancy.
  - removes the unused argument ''len'' to find_extent_buffer().

Signed-Off-by: Chandra Seetharaman <sekharan@us.ibm.com>
Reviewed-by: Zach Brown <zab@redhat.com>

---
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 4ae17ed..daa04d2 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -1105,8 +1105,7 @@ struct extent_buffer *btrfs_find_tree_block(struct
btrfs_root *root,
 {
 	struct inode *btree_inode = root->fs_info->btree_inode;
 	struct extent_buffer *eb;
-	eb = find_extent_buffer(&BTRFS_I(btree_inode)->io_tree,
-				bytenr, blocksize);
+	eb = find_extent_buffer(&BTRFS_I(btree_inode)->io_tree, bytenr);
 	return eb;
 }
 
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index c09a40d..f45b4e8 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)
+{
+	struct extent_buffer *eb;
+
+	rcu_read_lock();
+	eb = radix_tree_lookup(&tree->buffer, start >> PAGE_CACHE_SHIFT);
+	if (eb && atomic_inc_not_zero(&eb->refs)) {
+		rcu_read_unlock();
+		mark_extent_buffer_accessed(eb);
+		return eb;
+	}
+	rcu_read_unlock();
+
+	return NULL;
+}
+
 struct extent_buffer *alloc_extent_buffer(struct extent_io_tree *tree,
 					  u64 start, unsigned long len)
 {
@@ -4464,14 +4481,10 @@ struct extent_buffer *alloc_extent_buffer(struct
extent_io_tree *tree,
 	int uptodate = 1;
 	int ret;
 
-	rcu_read_lock();
-	eb = radix_tree_lookup(&tree->buffer, start >> PAGE_CACHE_SHIFT);
-	if (eb && atomic_inc_not_zero(&eb->refs)) {
-		rcu_read_unlock();
-		mark_extent_buffer_accessed(eb);
+
+	eb = find_extent_buffer(tree, start);
+	if (eb)
 		return eb;
-	}
-	rcu_read_unlock();
 
 	eb = __alloc_extent_buffer(tree, start, len, GFP_NOFS);
 	if (!eb)
@@ -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);
+		if (exists)
+			goto free_eb;
+		else
 			goto again;
-		}
-		spin_unlock(&tree->buffer_lock);
-		radix_tree_preload_end();
-		mark_extent_buffer_accessed(exists);
-		goto free_eb;
 	}
 	/* add one reference for the tree */
 	check_buffer_tree_ref(eb);
-	spin_unlock(&tree->buffer_lock);
-	radix_tree_preload_end();
 
 	/*
 	 * there is a race where release page may have
@@ -4578,23 +4584,6 @@ free_eb:
 	return exists;
 }
 
-struct extent_buffer *find_extent_buffer(struct extent_io_tree *tree,
-					 u64 start, unsigned long len)
-{
-	struct extent_buffer *eb;
-
-	rcu_read_lock();
-	eb = radix_tree_lookup(&tree->buffer, start >> PAGE_CACHE_SHIFT);
-	if (eb && atomic_inc_not_zero(&eb->refs)) {
-		rcu_read_unlock();
-		mark_extent_buffer_accessed(eb);
-		return eb;
-	}
-	rcu_read_unlock();
-
-	return NULL;
-}
-
 static inline void btrfs_release_extent_buffer_rcu(struct rcu_head *head)
 {
 	struct extent_buffer *eb diff --git a/fs/btrfs/extent_io.h
b/fs/btrfs/extent_io.h
index 6dbc645..a4b75af 100644
--- a/fs/btrfs/extent_io.h
+++ b/fs/btrfs/extent_io.h
@@ -271,7 +271,7 @@ struct extent_buffer *alloc_extent_buffer(struct
extent_io_tree *tree,
 struct extent_buffer *alloc_dummy_extent_buffer(u64 start, unsigned long len);
 struct extent_buffer *btrfs_clone_extent_buffer(struct extent_buffer *src);
 struct extent_buffer *find_extent_buffer(struct extent_io_tree *tree,
-					 u64 start, unsigned long len);
+					 u64 start);
 void free_extent_buffer(struct extent_buffer *eb);
 void free_extent_buffer_stale(struct extent_buffer *eb);
 #define WAIT_NONE	0


--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs"
in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Btrfs devel - Oct 2013 - [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case

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

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

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

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

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