Re: [PATCH] Btrfs: separate sequence numbers for delayed ref tracking and tree mod log
On Tue, Apr 23, 2013 at 08:00:27PM +0200, Jan Schmidt wrote:
Sequence numbers for delayed refs have been introduced in the first version
of the qgroup patch set. To solve the problem of find_all_roots on a busy
file system, the tree mod log was introduced. The sequence numbers for that
were simply shared between those two users.
Can't we just separate them with two vars?
thanks,
liubo
However, at one point in qgroup's quota accounting, there's a statement
accessing the previous sequence number, that's still just doing (seq - 1)
just as it had to in the very first version.
To satisfy that requirement, this patch makes the sequence number counter 64
bit and splits it into a major part (used for qgroup sequence number
counting) and a minor part (incremented for each tree modification in the
log). This enables us to go exactly one major step backwards, as required
for qgroups, while still incrementing the sequence counter for tree mod log
insertions to keep track of their order. Keeping them in a single variable
means there's no need to change all the code dealing with comparisons of two
sequence numbers.
The sequence number is reset to 0 on commit (not new in this patch), which
ensures we won't overflow the two 32 bit counters.
Without this fix, the qgroup tracking can occasionally go wrong and WARN_ONs
from the tree mod log code may happen.
Signed-off-by: Jan Schmidt list.bt...@jan-o-sch.net
---
fs/btrfs/ctree.c | 36 +---
fs/btrfs/ctree.h |7 ++-
fs/btrfs/delayed-ref.c |6 --
fs/btrfs/disk-io.c |2 +-
fs/btrfs/extent-tree.c |5 +++--
fs/btrfs/qgroup.c | 13 -
fs/btrfs/transaction.c |2 +-
7 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 566d99b..b74136e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -361,6 +361,36 @@ static inline void tree_mod_log_write_unlock(struct
btrfs_fs_info *fs_info)
}
/*
+ * increment the upper half of tree_mod_seq, set lower half zero
+ *
+ * must be called with fs_info-tree_mod_seq_lock held
+ */
+static inline u64 btrfs_inc_tree_mod_seq_major(struct btrfs_fs_info *fs_info)
+{
+ u64 seq = atomic64_read(fs_info-tree_mod_seq);
+ seq = 0xull;
+ seq += 1ull 32;
+ atomic64_set(fs_info-tree_mod_seq, seq);
+ return seq;
+}
+
+/*
+ * increment the lower half of tree_mod_seq
+ */
+static inline u64 btrfs_inc_tree_mod_seq_minor(struct btrfs_fs_info *fs_info)
+{
+ return atomic64_inc_return(fs_info-tree_mod_seq);
+}
+
+/*
+ * return the last minor in the previous major tree_mod_seq number
+ */
+u64 btrfs_tree_mod_seq_prev(u64 seq)
+{
+ return (seq 0xull) - 1ull;
+}
+
+/*
* This adds a new blocker to the tree mod log's blocker list if the @elem
* passed does not already have a sequence number set. So when a caller
expects
* to record tree modifications, it should ensure to set elem-seq to zero
@@ -376,10 +406,10 @@ u64 btrfs_get_tree_mod_seq(struct btrfs_fs_info
*fs_info,
tree_mod_log_write_lock(fs_info);
spin_lock(fs_info-tree_mod_seq_lock);
if (!elem-seq) {
- elem-seq = btrfs_inc_tree_mod_seq(fs_info);
+ elem-seq = btrfs_inc_tree_mod_seq_major(fs_info);
list_add_tail(elem-list, fs_info-tree_mod_seq_list);
}
- seq = btrfs_inc_tree_mod_seq(fs_info);
+ seq = btrfs_inc_tree_mod_seq_minor(fs_info);
spin_unlock(fs_info-tree_mod_seq_lock);
tree_mod_log_write_unlock(fs_info);
@@ -524,7 +554,7 @@ static inline int tree_mod_alloc(struct btrfs_fs_info
*fs_info, gfp_t flags,
if (!tm)
return -ENOMEM;
- tm-seq = btrfs_inc_tree_mod_seq(fs_info);
+ tm-seq = btrfs_inc_tree_mod_seq_minor(fs_info);
return tm-seq;
}
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 412c306..5f34f89 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1422,7 +1422,7 @@ struct btrfs_fs_info {
/* this protects tree_mod_seq_list */
spinlock_t tree_mod_seq_lock;
- atomic_t tree_mod_seq;
+ atomic64_t tree_mod_seq;
struct list_head tree_mod_seq_list;
struct seq_list tree_mod_seq_elem;
@@ -3334,10 +3334,7 @@ u64 btrfs_get_tree_mod_seq(struct btrfs_fs_info
*fs_info,
struct seq_list *elem);
void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
struct seq_list *elem);
-static inline u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
-{
- return atomic_inc_return(fs_info-tree_mod_seq);
-}
+u64 btrfs_tree_mod_seq_prev(u64 seq);
int btrfs_old_root_level(struct btrfs_root *root, u64 time_seq);
/* root-item.c */
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 116abec..c219463 100644
--- a/fs/btrfs/delayed-ref.c
Re: [PATCH] Btrfs: separate sequence numbers for delayed ref tracking and tree mod log
On Wed, April 24, 2013 at 10:12 (+0200), Liu Bo wrote: On Tue, Apr 23, 2013 at 08:00:27PM +0200, Jan Schmidt wrote: Sequence numbers for delayed refs have been introduced in the first version of the qgroup patch set. To solve the problem of find_all_roots on a busy file system, the tree mod log was introduced. The sequence numbers for that were simply shared between those two users. Can't we just separate them with two vars? My reasoning comes a few lines below ... thanks, liubo However, at one point in qgroup's quota accounting, there's a statement accessing the previous sequence number, that's still just doing (seq - 1) just as it had to in the very first version. To satisfy that requirement, this patch makes the sequence number counter 64 bit and splits it into a major part (used for qgroup sequence number counting) and a minor part (incremented for each tree modification in the log). This enables us to go exactly one major step backwards, as required for qgroups, while still incrementing the sequence counter for tree mod log insertions to keep track of their order. Keeping them in a single variable means there's no need to change all the code dealing with comparisons of two sequence numbers. See the previous sentence :-) And, it doesn't add too much complexity, setting and incrementing remains in fact quite easy, even though we use the upper 32 bit and the lower 32 bit of that integer independently. Thanks, -Jan The sequence number is reset to 0 on commit (not new in this patch), which ensures we won't overflow the two 32 bit counters. Without this fix, the qgroup tracking can occasionally go wrong and WARN_ONs from the tree mod log code may happen. Signed-off-by: Jan Schmidt list.bt...@jan-o-sch.net --- [snip] -- 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] Btrfs: separate sequence numbers for delayed ref tracking and tree mod log
On Tue, Apr 23, 2013 at 12:00:27PM -0600, Jan Schmidt wrote:
Sequence numbers for delayed refs have been introduced in the first version
of the qgroup patch set. To solve the problem of find_all_roots on a busy
file system, the tree mod log was introduced. The sequence numbers for that
were simply shared between those two users.
However, at one point in qgroup's quota accounting, there's a statement
accessing the previous sequence number, that's still just doing (seq - 1)
just as it had to in the very first version.
To satisfy that requirement, this patch makes the sequence number counter 64
bit and splits it into a major part (used for qgroup sequence number
counting) and a minor part (incremented for each tree modification in the
log). This enables us to go exactly one major step backwards, as required
for qgroups, while still incrementing the sequence counter for tree mod log
insertions to keep track of their order. Keeping them in a single variable
means there's no need to change all the code dealing with comparisons of two
sequence numbers.
The sequence number is reset to 0 on commit (not new in this patch), which
ensures we won't overflow the two 32 bit counters.
Without this fix, the qgroup tracking can occasionally go wrong and WARN_ONs
from the tree mod log code may happen.
Signed-off-by: Jan Schmidt list.bt...@jan-o-sch.net
---
fs/btrfs/ctree.c | 36 +---
fs/btrfs/ctree.h |7 ++-
fs/btrfs/delayed-ref.c |6 --
fs/btrfs/disk-io.c |2 +-
fs/btrfs/extent-tree.c |5 +++--
fs/btrfs/qgroup.c | 13 -
fs/btrfs/transaction.c |2 +-
7 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 566d99b..b74136e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -361,6 +361,36 @@ static inline void tree_mod_log_write_unlock(struct
btrfs_fs_info *fs_info)
}
/*
+ * increment the upper half of tree_mod_seq, set lower half zero
+ *
+ * must be called with fs_info-tree_mod_seq_lock held
+ */
+static inline u64 btrfs_inc_tree_mod_seq_major(struct btrfs_fs_info *fs_info)
+{
+ u64 seq = atomic64_read(fs_info-tree_mod_seq);
+ seq = 0xull;
+ seq += 1ull 32;
+ atomic64_set(fs_info-tree_mod_seq, seq);
+ return seq;
+}
This isn't going to work, you read in the value, inc it and then set the new
value. If somebody comes in and inc's in between the read and the sync, like
btrfs_inc_tree_mod_seq_minor could do when you call tree_mod_alloc, you'll end
up losing the minor update. Thanks,
Josef
--
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] Btrfs: separate sequence numbers for delayed ref tracking and tree mod log
On Wed, April 24, 2013 at 15:04 (+0200), Josef Bacik wrote:
On Tue, Apr 23, 2013 at 12:00:27PM -0600, Jan Schmidt wrote:
Sequence numbers for delayed refs have been introduced in the first version
of the qgroup patch set. To solve the problem of find_all_roots on a busy
file system, the tree mod log was introduced. The sequence numbers for that
were simply shared between those two users.
However, at one point in qgroup's quota accounting, there's a statement
accessing the previous sequence number, that's still just doing (seq - 1)
just as it had to in the very first version.
To satisfy that requirement, this patch makes the sequence number counter 64
bit and splits it into a major part (used for qgroup sequence number
counting) and a minor part (incremented for each tree modification in the
log). This enables us to go exactly one major step backwards, as required
for qgroups, while still incrementing the sequence counter for tree mod log
insertions to keep track of their order. Keeping them in a single variable
means there's no need to change all the code dealing with comparisons of two
sequence numbers.
The sequence number is reset to 0 on commit (not new in this patch), which
ensures we won't overflow the two 32 bit counters.
Without this fix, the qgroup tracking can occasionally go wrong and WARN_ONs
from the tree mod log code may happen.
Signed-off-by: Jan Schmidt list.bt...@jan-o-sch.net
---
fs/btrfs/ctree.c | 36 +---
fs/btrfs/ctree.h |7 ++-
fs/btrfs/delayed-ref.c |6 --
fs/btrfs/disk-io.c |2 +-
fs/btrfs/extent-tree.c |5 +++--
fs/btrfs/qgroup.c | 13 -
fs/btrfs/transaction.c |2 +-
7 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 566d99b..b74136e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -361,6 +361,36 @@ static inline void tree_mod_log_write_unlock(struct
btrfs_fs_info *fs_info)
}
/*
+ * increment the upper half of tree_mod_seq, set lower half zero
+ *
+ * must be called with fs_info-tree_mod_seq_lock held
+ */
+static inline u64 btrfs_inc_tree_mod_seq_major(struct btrfs_fs_info
*fs_info)
+{
+u64 seq = atomic64_read(fs_info-tree_mod_seq);
+seq = 0xull;
+seq += 1ull 32;
+atomic64_set(fs_info-tree_mod_seq, seq);
+return seq;
+}
This isn't going to work, you read in the value, inc it and then set the new
value. If somebody comes in and inc's in between the read and the sync, like
btrfs_inc_tree_mod_seq_minor could do when you call tree_mod_alloc, you'll end
up losing the minor update. Thanks,
I don't think I'll lose it. The minor update is made and returned to the one who
needs it, that number can still be used. There is no guarantee for two
concurrent modifications to which major a minor number belongs, though.
Thanks,
-Jan
--
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] Btrfs: separate sequence numbers for delayed ref tracking and tree mod log
On Wed, Apr 24, 2013 at 07:25:09AM -0600, Jan Schmidt wrote:
On Wed, April 24, 2013 at 15:04 (+0200), Josef Bacik wrote:
On Tue, Apr 23, 2013 at 12:00:27PM -0600, Jan Schmidt wrote:
Sequence numbers for delayed refs have been introduced in the first version
of the qgroup patch set. To solve the problem of find_all_roots on a busy
file system, the tree mod log was introduced. The sequence numbers for that
were simply shared between those two users.
However, at one point in qgroup's quota accounting, there's a statement
accessing the previous sequence number, that's still just doing (seq - 1)
just as it had to in the very first version.
To satisfy that requirement, this patch makes the sequence number counter
64
bit and splits it into a major part (used for qgroup sequence number
counting) and a minor part (incremented for each tree modification in the
log). This enables us to go exactly one major step backwards, as required
for qgroups, while still incrementing the sequence counter for tree mod log
insertions to keep track of their order. Keeping them in a single variable
means there's no need to change all the code dealing with comparisons of
two
sequence numbers.
The sequence number is reset to 0 on commit (not new in this patch), which
ensures we won't overflow the two 32 bit counters.
Without this fix, the qgroup tracking can occasionally go wrong and
WARN_ONs
from the tree mod log code may happen.
Signed-off-by: Jan Schmidt list.bt...@jan-o-sch.net
---
fs/btrfs/ctree.c | 36 +---
fs/btrfs/ctree.h |7 ++-
fs/btrfs/delayed-ref.c |6 --
fs/btrfs/disk-io.c |2 +-
fs/btrfs/extent-tree.c |5 +++--
fs/btrfs/qgroup.c | 13 -
fs/btrfs/transaction.c |2 +-
7 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 566d99b..b74136e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -361,6 +361,36 @@ static inline void tree_mod_log_write_unlock(struct
btrfs_fs_info *fs_info)
}
/*
+ * increment the upper half of tree_mod_seq, set lower half zero
+ *
+ * must be called with fs_info-tree_mod_seq_lock held
+ */
+static inline u64 btrfs_inc_tree_mod_seq_major(struct btrfs_fs_info
*fs_info)
+{
+ u64 seq = atomic64_read(fs_info-tree_mod_seq);
+ seq = 0xull;
+ seq += 1ull 32;
+ atomic64_set(fs_info-tree_mod_seq, seq);
+ return seq;
+}
This isn't going to work, you read in the value, inc it and then set the new
value. If somebody comes in and inc's in between the read and the sync,
like
btrfs_inc_tree_mod_seq_minor could do when you call tree_mod_alloc, you'll
end
up losing the minor update. Thanks,
I don't think I'll lose it. The minor update is made and returned to the one
who
needs it, that number can still be used. There is no guarantee for two
concurrent modifications to which major a minor number belongs, though.
Ah yeah it's returned, but then you'll end up with two things that have the same
seq number when they shouldn't have. I'm not sure if that's a bad thing or not,
but from up here it looks like a bad thing, at the very least it's going to
surprise anybody who goes to work on this code later and expects it to work out
like a normal atomic would. So either wrap it in a spin_lock (it looks like you
already have a mod_seq_lock) or come up with something less crafty. Thanks,
Josef
--
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] Btrfs: separate sequence numbers for delayed ref tracking and tree mod log
Sequence numbers for delayed refs have been introduced in the first version
of the qgroup patch set. To solve the problem of find_all_roots on a busy
file system, the tree mod log was introduced. The sequence numbers for that
were simply shared between those two users.
However, at one point in qgroup's quota accounting, there's a statement
accessing the previous sequence number, that's still just doing (seq - 1)
just as it had to in the very first version.
To satisfy that requirement, this patch makes the sequence number counter 64
bit and splits it into a major part (used for qgroup sequence number
counting) and a minor part (incremented for each tree modification in the
log). This enables us to go exactly one major step backwards, as required
for qgroups, while still incrementing the sequence counter for tree mod log
insertions to keep track of their order. Keeping them in a single variable
means there's no need to change all the code dealing with comparisons of two
sequence numbers.
The sequence number is reset to 0 on commit (not new in this patch), which
ensures we won't overflow the two 32 bit counters.
Without this fix, the qgroup tracking can occasionally go wrong and WARN_ONs
from the tree mod log code may happen.
Signed-off-by: Jan Schmidt list.bt...@jan-o-sch.net
---
fs/btrfs/ctree.c | 36 +---
fs/btrfs/ctree.h |7 ++-
fs/btrfs/delayed-ref.c |6 --
fs/btrfs/disk-io.c |2 +-
fs/btrfs/extent-tree.c |5 +++--
fs/btrfs/qgroup.c | 13 -
fs/btrfs/transaction.c |2 +-
7 files changed, 52 insertions(+), 19 deletions(-)
diff --git a/fs/btrfs/ctree.c b/fs/btrfs/ctree.c
index 566d99b..b74136e 100644
--- a/fs/btrfs/ctree.c
+++ b/fs/btrfs/ctree.c
@@ -361,6 +361,36 @@ static inline void tree_mod_log_write_unlock(struct
btrfs_fs_info *fs_info)
}
/*
+ * increment the upper half of tree_mod_seq, set lower half zero
+ *
+ * must be called with fs_info-tree_mod_seq_lock held
+ */
+static inline u64 btrfs_inc_tree_mod_seq_major(struct btrfs_fs_info *fs_info)
+{
+ u64 seq = atomic64_read(fs_info-tree_mod_seq);
+ seq = 0xull;
+ seq += 1ull 32;
+ atomic64_set(fs_info-tree_mod_seq, seq);
+ return seq;
+}
+
+/*
+ * increment the lower half of tree_mod_seq
+ */
+static inline u64 btrfs_inc_tree_mod_seq_minor(struct btrfs_fs_info *fs_info)
+{
+ return atomic64_inc_return(fs_info-tree_mod_seq);
+}
+
+/*
+ * return the last minor in the previous major tree_mod_seq number
+ */
+u64 btrfs_tree_mod_seq_prev(u64 seq)
+{
+ return (seq 0xull) - 1ull;
+}
+
+/*
* This adds a new blocker to the tree mod log's blocker list if the @elem
* passed does not already have a sequence number set. So when a caller expects
* to record tree modifications, it should ensure to set elem-seq to zero
@@ -376,10 +406,10 @@ u64 btrfs_get_tree_mod_seq(struct btrfs_fs_info *fs_info,
tree_mod_log_write_lock(fs_info);
spin_lock(fs_info-tree_mod_seq_lock);
if (!elem-seq) {
- elem-seq = btrfs_inc_tree_mod_seq(fs_info);
+ elem-seq = btrfs_inc_tree_mod_seq_major(fs_info);
list_add_tail(elem-list, fs_info-tree_mod_seq_list);
}
- seq = btrfs_inc_tree_mod_seq(fs_info);
+ seq = btrfs_inc_tree_mod_seq_minor(fs_info);
spin_unlock(fs_info-tree_mod_seq_lock);
tree_mod_log_write_unlock(fs_info);
@@ -524,7 +554,7 @@ static inline int tree_mod_alloc(struct btrfs_fs_info
*fs_info, gfp_t flags,
if (!tm)
return -ENOMEM;
- tm-seq = btrfs_inc_tree_mod_seq(fs_info);
+ tm-seq = btrfs_inc_tree_mod_seq_minor(fs_info);
return tm-seq;
}
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index 412c306..5f34f89 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -1422,7 +1422,7 @@ struct btrfs_fs_info {
/* this protects tree_mod_seq_list */
spinlock_t tree_mod_seq_lock;
- atomic_t tree_mod_seq;
+ atomic64_t tree_mod_seq;
struct list_head tree_mod_seq_list;
struct seq_list tree_mod_seq_elem;
@@ -3334,10 +3334,7 @@ u64 btrfs_get_tree_mod_seq(struct btrfs_fs_info *fs_info,
struct seq_list *elem);
void btrfs_put_tree_mod_seq(struct btrfs_fs_info *fs_info,
struct seq_list *elem);
-static inline u64 btrfs_inc_tree_mod_seq(struct btrfs_fs_info *fs_info)
-{
- return atomic_inc_return(fs_info-tree_mod_seq);
-}
+u64 btrfs_tree_mod_seq_prev(u64 seq);
int btrfs_old_root_level(struct btrfs_root *root, u64 time_seq);
/* root-item.c */
diff --git a/fs/btrfs/delayed-ref.c b/fs/btrfs/delayed-ref.c
index 116abec..c219463 100644
--- a/fs/btrfs/delayed-ref.c
+++ b/fs/btrfs/delayed-ref.c
@@ -361,8 +361,10 @@ int btrfs_check_delayed_seq(struct btrfs_fs_info *fs_info,
elem = list_first_entry(fs_info-tree_mod_seq_list,
