[Devel] [PATCH RHEL7 COMMIT] fuse kio: Change order around pcs_map_notify_addr_change()

[Konstantin Khorenko]

The commit is pushed to "branch-rh7-3.10.0-693.21.1.vz7.50.x-ovz" and will appear at https://src.openvz.org/scm/ovz/vzkernel.git
after rh7-3.10.0-693.21.1.vz7.50.4
------>
commit 885971b6a2d878fff83b5709a96b5d6171fbab7d
Author: Kirill Tkhai <ktkhai at virtuozzo.com>
Date:   Mon Jun 4 23:24:15 2018 +0300

    fuse kio: Change order around pcs_map_notify_addr_change()
    
    This unblacklistes a cs after all maps are marked as stale,
    while rpc addr becomes assigned earlier.
    
    Since pcs_rpc::addr is dereferenced with cs unlocked,
    it seems, we can may such the change.
    This will be used in next patch.
    
    Signed-off-by: Kirill Tkhai <ktkhai at virtuozzo.com>
    
    =====================
    Patchset description:
    
    Fix deadlock during change of CS address
    
    This is not a complete patchset, but I meet the situation
    when it's necessary to change original logic in small way,
    so this is a request for comments.
    
    [1-5/7] are mostly preparations and fixes, so my question
    is about [6-7/7].
    
    1) Patch 6 changes order of actions: pcs_map_notify_addr_change()
    is called after assigning of rpc addr. Can we do that? As I
    understand this results in new maps are created with new address,
    while in pcs_map_notify_addr_change() we invalidate old ones.
    So, for me it seems there is no a problem.
    
    This is needed for possibility to unlock cs->lock in pcs_map_notify_addr_change().
    Theoretically, two pcs_cs_find_create() may happen in parallel,
    so we want they assign addr in the order they happen. Otherwise,
    the first one with the old addr_serno may overwrite the addr.
    
    2) Patch 7 uses the preparations from previous patches and
    makes pcs_map_notify_addr_change() to unlock cs->lock for a while.
    New elements are added to head of cs->map_list, so we skip
    them on iterations. But it seems, they must be correct because
    we already updated rpc addr in pcs_cs_find_create(). Is there
    a reason we can't do this?
    
    Kirill Tkhai (7):
          fuse kio: Introduce pcs_cs_list_of_cs_link()
          fuse kio: Fix potential use after free
          fuse kio: Fix possible use after free in cslist_destroy()
          fuse kio: Introduce pcs_cs::use_count instead of ::is_probing
          fuse kio: Wait till cs is unused in pcs_csset_fini()
          fuse kio: Change order around pcs_map_notify_addr_change()
          fuse kio: Fix fix deadlock during change CS address
---
 fs/fuse/kio/pcs/pcs_cs.c  | 7 +++++--
 fs/fuse/kio/pcs/pcs_map.c | 2 --
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/kio/pcs/pcs_cs.c b/fs/fuse/kio/pcs/pcs_cs.c
index 9614f2473629..27f0bc3754d9 100644
--- a/fs/fuse/kio/pcs/pcs_cs.c
+++ b/fs/fuse/kio/pcs/pcs_cs.c
@@ -193,11 +193,14 @@ struct pcs_cs *pcs_cs_find_create(struct pcs_cs_set *csset, PCS_NODE_ID_T *id, P
 			if (pcs_netaddr_cmp(&cs->addr, addr)) {
 				cs->addr = *addr;
 				cs->addr_serno++;
-				if (!(flags & CS_FL_INACTIVE))
-					pcs_map_notify_addr_change(cs);
+
 				TRACE("Port change CS" NODE_FMT " seq=%d", NODE_ARGS(*id), cs->addr_serno);
 				pcs_rpc_set_address(cs->rpc, addr);
 
+				if (!(flags & CS_FL_INACTIVE)) {
+					pcs_map_notify_addr_change(cs);
+					cs_whitelist(cs, "addr update");
+				}
 			}
 		}
 		/* TODO: (flags & PCS_RPC_F_LOCAL) should be checker here */
diff --git a/fs/fuse/kio/pcs/pcs_map.c b/fs/fuse/kio/pcs/pcs_map.c
index 56a1118af255..49af8b961478 100644
--- a/fs/fuse/kio/pcs/pcs_map.c
+++ b/fs/fuse/kio/pcs/pcs_map.c
@@ -699,8 +699,6 @@ void pcs_map_notify_addr_change(struct pcs_cs * cs)
 	struct pcs_cs_link * csl;
 	assert_spin_locked(&cs->lock);
 
-	cs_whitelist(cs, "addr update");
-
 	rcu_read_lock();
 	list_for_each_entry(csl, &cs->map_list, link) {
 		struct pcs_cs_list *cs_list;