The only purpose of rcu_read_lock() would be to ensure the object
isn't freed out from under you. That's not an issue here.

Racing with other writers isn't a correctness issue here, and if it
was - just throwing write_lock around where you have it wouldn't be
sufficient, you'd need something much more complicated.

On Wed, Jan 24, 2018 at 8:50 PM,  [off-list ref] wrote:

From: Tang Junhui <redacted>

Hello Kent && Nix

quoted

neither of those locks are needed - rcu_read_lock() isn't needed because we never
free struct btree (except at shutdown), and we're not derefing journal there

__bch_btree_node_write() is called in many places, in __bch_btree_node_write(),
before node writing over, it has changed the write buff index to another, but the
journal of changed write buff maybe still NULL if no journal writing occurred. But
luckily we only use the NULL as ZERO to calculate the fifo_idx, the result is a very
big value, so it does no harm. And since we cannot take mutex under rcu_read_lock,
we can ignore it.

As to the rcu lock, though the btree is not freed, but we often delete it from one
list (such as one bucket_hash) and move it to another list (such as btree_cache_freed),
shouldn't it be protected by rcu locker?

quoted

On Wed, Jan 24, 2018 at 5:30 AM, Nikolay Borisov [off-list ref] wrote:


On 24.01.2018 08:54, tang.junhui@zte.com.cn wrote:

quoted

From: Tang Junhui <redacted>

In btree_flush_write(), two places need to take a locker to
avoid races:

Firstly, we need take rcu read locker to protect the bucket_hash
traverse, since hlist_for_each_entry_rcu() must be called under
the protection of rcu read locker.

Secondly, we need take b->write_lock locker to protect journal
of the btree node, otherwise, the btree node may have been
written, and the journal have been assign to NULL.

Signed-off-by: Tang Junhui <redacted>
---
 drivers/md/bcache/journal.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/bcache/journal.c b/drivers/md/bcache/journal.c

index 02a98dd.505f9f3 100644

--- a/drivers/md/bcache/journal.c
+++ b/drivers/md/bcache/journal.c

@@ -375,7 +375,9 @@ static void btree_flush_write(struct cache_set *c)
 retry:
      best = NULL;

-     for_each_cached_btree(b, c, i)
+     rcu_read_lock();
+     for_each_cached_btree(b, c, i) {
+             mutex_lock(&b->write_lock);

quoted

You can't sleep in rcu read critical section, yet here you take mutex
which can sleep under rcu_read_lock.

To Nix: Yes, you are write. Good catch.

quoted

quoted

              if (btree_current_write(b)->journal) {
                      if (!best)
                              best = b;

@@ -385,6 +387,9 @@ static void btree_flush_write(struct cache_set *c)
                              best = b;
                      }
              }
+             mutex_unlock(&b->write_lock);
+     }
+     rcu_read_unlock();

      b = best;
      if (b) {

Thanks,
Tang Junhui