Re: [PATCH v0 03/18] btrfs: add nested locking mode for paths

[Arne Jansen]

On 06.10.2011 22:54, Andrey Kuzmin wrote:
> 
> 
> On Friday, October 7, 2011, Andi Kleen  > wrote:
>> On Fri, Oct 07, 2011 at 12:44:30AM +0400, Andrey Kuzmin wrote:
>>> Perhaps you could just elaborate on "needs this feature"? In general, write
>>> lock gives one exclusive access, so the need for additional read
>>> (non-exclusive) lock does not appear easily understandable.
>>
>> Usually it's because the low level code can be called both with and
>> without locking and it doesn't know.
>>
>> But that usually can be avoided with some restructuring.
> Exactly.

One premise was to change the existing paths as little as possible to
make it easier for Chris to judge the impact when qgroups are turned
off. For this I hook into the generic "add a backref" path, which is
called from diverse places all around the code. Some of these places
already hold a lock on the fs tree I'm going to read, others don't,
but in any case it is ok to just go ahead and read it.
I think by restructuring you mean serializing both operations and only
calling into my code after the previous operation is done. But that
would on one hand mean queuing up all that needs to be done afterwards,
and on the other hand at least keeping a read lock on all these extents,
as no changes are allowed in between.
This restructuring would be a major operation, but maybe I'm just not
seeing the obvious way to go. Maybe Chris has a good idea...

All my code is now doing is when it encounters a write lock, check if the
upper layers are holding it and allowing read access if that's the case.
Only one additional reader is allowed. This is not full recursive locking,
but at least part of it.

-Arne

>>
>> -Andi
>>
> 
> -- 
> Sent from Gmail Mobile

--
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

Re: [PATCH v0 03/18] btrfs: add nested locking mode for paths

[Andi Kleen]

On Fri, Oct 07, 2011 at 12:44:30AM +0400, Andrey Kuzmin wrote:
> Perhaps you could just elaborate on "needs this feature"? In general, write
> lock gives one exclusive access, so the need for additional read
> (non-exclusive) lock does not appear easily understandable.

Usually it's because the low level code can be called both with and
without locking and it doesn't know.

But that usually can be avoided with some restructuring.

-Andi
--
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

Re: [PATCH v0 03/18] btrfs: add nested locking mode for paths

[Andi Kleen]

Arne Jansen  writes:

> This patch adds the possibilty to read-lock an extent
> even if it is already write-locked from the same thread.
> Subvolume quota needs this capability.

Recursive locking is generally strongly discouraged, it causes all kinds
of problems and tends to eventuall ylead to locking hierarchies nobody
can understand anymore.

If you can find any other way to solve this problem I would
encourage you to do so.

-Andi

-- 
a...@linux.intel.com -- Speaking for myself only
--
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

[PATCH v0 03/18] btrfs: add nested locking mode for paths

[Arne Jansen]

This patch adds the possibilty to read-lock an extent
even if it is already write-locked from the same thread.
Subvolume quota needs this capability.

Signed-off-by: Arne Jansen 
---
 fs/btrfs/ctree.c |   22 
 fs/btrfs/ctree.h |1 +
 fs/btrfs/extent_io.c |1 +
 fs/btrfs/extent_io.h |2 +
 fs/btrfs/locking.c   |   51 +++--
 fs/btrfs/locking.h   |2 +-
 6 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 51b387b..964ac9a 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -186,13 +186,14 @@ struct extent_buffer *btrfs_lock_root_node(struct 
btrfs_root *root)
  * tree until you end up with a lock on the root.  A locked buffer
  * is returned, with a reference held.
  */
-struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root)
+struct extent_buffer *btrfs_read_lock_root_node(struct btrfs_root *root,
+   int nested)
 {
struct extent_buffer *eb;
 
while (1) {
eb = btrfs_root_node(root);
-   btrfs_tree_read_lock(eb);
+   btrfs_tree_read_lock(eb, nested);
if (eb == root->node)
break;
btrfs_tree_read_unlock(eb);
@@ -1620,6 +1621,7 @@ int btrfs_search_slot(struct btrfs_trans_handle *trans, 
struct btrfs_root
/* everything at write_lock_level or lower must be write locked */
int write_lock_level = 0;
u8 lowest_level = 0;
+   int nested = p->nested;
 
lowest_level = p->lowest_level;
WARN_ON(lowest_level && ins_len > 0);
@@ -1661,8 +1663,9 @@ again:
b = root->commit_root;
extent_buffer_get(b);
level = btrfs_header_level(b);
+   BUG_ON(p->skip_locking && nested);
if (!p->skip_locking)
-   btrfs_tree_read_lock(b);
+   btrfs_tree_read_lock(b, 0);
} else {
if (p->skip_locking) {
b = btrfs_root_node(root);
@@ -1671,7 +1674,7 @@ again:
/* we don't know the level of the root node
 * until we actually have it read locked
 */
-   b = btrfs_read_lock_root_node(root);
+   b = btrfs_read_lock_root_node(root, nested);
level = btrfs_header_level(b);
if (level <= write_lock_level) {
/* whoops, must trade for write lock */
@@ -1810,7 +1813,8 @@ cow_done:
err = btrfs_try_tree_read_lock(b);
if (!err) {
btrfs_set_path_blocking(p);
-   btrfs_tree_read_lock(b);
+   btrfs_tree_read_lock(b,
+nested);
btrfs_clear_path_blocking(p, b,
  
BTRFS_READ_LOCK);
}
@@ -3955,7 +3959,7 @@ int btrfs_search_forward(struct btrfs_root *root, struct 
btrfs_key *min_key,
 
WARN_ON(!path->keep_locks);
 again:
-   cur = btrfs_read_lock_root_node(root);
+   cur = btrfs_read_lock_root_node(root, 0);
level = btrfs_header_level(cur);
WARN_ON(path->nodes[level]);
path->nodes[level] = cur;
@@ -4049,7 +4053,7 @@ find_next_key:
cur = read_node_slot(root, cur, slot);
BUG_ON(!cur);
 
-   btrfs_tree_read_lock(cur);
+   btrfs_tree_read_lock(cur, 0);
 
path->locks[level - 1] = BTRFS_READ_LOCK;
path->nodes[level - 1] = cur;
@@ -4243,7 +4247,7 @@ again:
ret = btrfs_try_tree_read_lock(next);
if (!ret) {
btrfs_set_path_blocking(path);
-   btrfs_tree_read_lock(next);
+   btrfs_tree_read_lock(next, 0);
btrfs_clear_path_blocking(path, next,
  BTRFS_READ_LOCK);
}
@@ -4280,7 +4284,7 @@ again:
ret = btrfs_try_tree_read_lock(next);
if (!ret) {
btrfs_set_path_blocking(path);
-   btrfs_tree_read_lock(next);
+   btrfs_tree_read_lock(next, 0);
btrfs_clear_path_blocking(path, next,
  BTRFS_READ_LOCK);
}
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree