* [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case
@ 2013-10-04 19:55 Chandra Seetharaman
2013-10-04 21:38 ` Zach Brown
0 siblings, 1 reply; 5+ messages in thread
From: Chandra Seetharaman @ 2013-10-04 19:55 UTC (permalink / raw)
To: Btrfs mailing list
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 =
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case
2013-10-04 19:55 Chandra Seetharaman
@ 2013-10-04 21:38 ` Zach Brown
2013-10-04 22:00 ` Chandra Seetharaman
0 siblings, 1 reply; 5+ messages in thread
From: Zach Brown @ 2013-10-04 21:38 UTC (permalink / raw)
To: Chandra Seetharaman; +Cc: Btrfs mailing list
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case
2013-10-04 21:38 ` Zach Brown
@ 2013-10-04 22:00 ` Chandra Seetharaman
2013-10-04 22:09 ` Zach Brown
0 siblings, 1 reply; 5+ messages in thread
From: Chandra Seetharaman @ 2013-10-04 22:00 UTC (permalink / raw)
To: Zach Brown; +Cc: Btrfs mailing list
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
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case
2013-10-04 22:00 ` Chandra Seetharaman
@ 2013-10-04 22:09 ` Zach Brown
0 siblings, 0 replies; 5+ messages in thread
From: Zach Brown @ 2013-10-04 22:09 UTC (permalink / raw)
To: Chandra Seetharaman; +Cc: Btrfs mailing list
> > > +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
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case
@ 2013-10-07 15:45 Chandra Seetharaman
0 siblings, 0 replies; 5+ messages in thread
From: Chandra Seetharaman @ 2013-10-07 15:45 UTC (permalink / raw)
To: Btrfs mailing list; +Cc: Zach Brown
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-10-07 15:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-10-07 15:45 [PATCH] Btrfs: Simplify the logic in alloc_extent_buffer() for existing extent buffer case Chandra Seetharaman
-- strict thread matches above, loose matches on Subject: below --
2013-10-04 19:55 Chandra Seetharaman
2013-10-04 21:38 ` Zach Brown
2013-10-04 22:00 ` Chandra Seetharaman
2013-10-04 22:09 ` Zach Brown
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).