Bug 1500472 - Use a bitmap to store local node info instead of conf->local_nodeuuids[i].uuids
Summary: Use a bitmap to store local node info instead of conf->local_nodeuuids[i].uuids
Keywords:
Status: CLOSED CURRENTRELEASE
Alias: None
Product: GlusterFS
Classification: Community
Component: distribute
Version: 3.12
Hardware: Unspecified
OS: Unspecified
high
unspecified
Target Milestone: ---
Assignee: bugs@gluster.org
QA Contact:
URL:
Whiteboard:
Depends On: 1451434
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-10-10 16:15 UTC by Nithya Balachandran
Modified: 2017-10-13 12:47 UTC (History)
2 users (show)

Fixed In Version: glusterfs-glusterfs-3.12.2
Doc Type: If docs needed, set a value
Doc Text:
Clone Of: 1451434
Environment:
Last Closed: 2017-10-13 12:47:15 UTC
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Embargoed:


Attachments (Terms of Use)

Description Nithya Balachandran 2017-10-10 16:15:34 UTC
+++ This bug was initially created as a clone of Bug #1451434 +++

Description of problem:
From Jeff Darcy's review:

"Do we really need to gf_uuid_compare here?  What if, instead of storing the complete list of UUIDs for each replica set, we instead stored a bitmap of which replicas were local?  Then this comparison would look more like this.
   if ((1 << index) && conf->local_replicas_mask)
Because the bitmap would be more compact, this might also ease worries about dynamic memory allocation etc."

...

"I'm just keenly aware that this is likely to be a frequently executed code path - there could be millions of files to be rebalance - so extra loops need to be carefully considered.  If they're justified, great, so long as they're considered."

Version-Release number of selected component (if applicable):


How reproducible:


Steps to Reproduce:
1.
2.
3.

Actual results:


Expected results:


Additional info:

--- Additional comment from Worker Ant on 2017-07-21 07:15:49 EDT ---

REVIEW: https://review.gluster.org/17851 (cluster/dht: Don't store the entire uuid for subvols WIP) posted (#1) for review on master by N Balachandran (nbalacha)

--- Additional comment from Worker Ant on 2017-09-15 02:07:26 EDT ---

REVIEW: https://review.gluster.org/17851 (cluster/dht: Don't store the entire uuid for subvols) posted (#2) for review on master by N Balachandran (nbalacha)

--- Additional comment from Worker Ant on 2017-09-15 12:20:07 EDT ---

REVIEW: https://review.gluster.org/17851 (cluster/dht: Don't store the entire uuid for subvols) posted (#3) for review on master by N Balachandran (nbalacha)

--- Additional comment from Worker Ant on 2017-10-03 10:00:31 EDT ---

REVIEW: https://review.gluster.org/17851 (cluster/dht: Don't store the entire uuid for subvols) posted (#4) for review on master by N Balachandran (nbalacha)

--- Additional comment from Worker Ant on 2017-10-04 03:45:00 EDT ---

REVIEW: https://review.gluster.org/17851 (cluster/dht: Don't store the entire uuid for subvols) posted (#5) for review on master by N Balachandran (nbalacha)

--- Additional comment from Worker Ant on 2017-10-05 08:39:36 EDT ---

REVIEW: https://review.gluster.org/17851 (cluster/dht: Don't store the entire uuid for subvols) posted (#6) for review on master by N Balachandran (nbalacha)

--- Additional comment from Worker Ant on 2017-10-10 04:58:43 EDT ---

COMMIT: https://review.gluster.org/17851 committed in master by Raghavendra G (rgowdapp) 
------
commit c4a608799a577a4f38139f6bb8a47da8efb0fec3
Author: N Balachandran <nbalacha>
Date:   Fri Jul 21 16:38:14 2017 +0530

    cluster/dht: Don't store the entire uuid for subvols
    
    Comparing the uuid string of the local node against that stored in the
    local_subvol information is inefficient, especially as it is
    done for every file to be migrated. The code has now been changed
    to set the value of info to 1 if the nodeuuid is that of the node
    making the comparison so this becomes an integer comparison.
    
    Change-Id: I7491d59caad3b71dbf5facc94dcde0cd53962775
    BUG: 1451434
    Signed-off-by: N Balachandran <nbalacha>

Comment 1 Worker Ant 2017-10-10 16:17:33 UTC
REVIEW: https://review.gluster.org/18480 (cluster/dht: Don't store the entire uuid for subvols) posted (#1) for review on release-3.12 by N Balachandran (nbalacha)

Comment 2 Worker Ant 2017-10-12 18:25:33 UTC
COMMIT: https://review.gluster.org/18480 committed in release-3.12 by jiffin tony Thottan (jthottan) 
------
commit 417088fbac3e44d87802cfc988a51e2ad7968d70
Author: N Balachandran <nbalacha>
Date:   Tue Oct 10 21:45:01 2017 +0530

    cluster/dht: Don't store the entire uuid for subvols
    
    Comparing the uuid string of the local node against that stored in the
    local_subvol information is inefficient, especially as it is
    done for every file to be migrated. The code has now been changed
    to set the value of info to 1 if the nodeuuid is that of the node
    making the comparison so this becomes an integer comparison.
    
    > BUG: 1451434
    > Signed-off-by: N Balachandran <nbalacha>
    > https://review.gluster.org/#/c/17851
    
    (cherry picked from commit c4a608799a577a4f38139f6bb8a47da8efb0fec3)
    Change-Id: I7491d59caad3b71dbf5facc94dcde0cd53962775
    BUG: 1500472
    Signed-off-by: N Balachandran <nbalacha>

Comment 3 Jiffin 2017-10-13 12:47:15 UTC
This bug is getting closed because a release has been made available that should address the reported issue. In case the problem is still not fixed with glusterfs-glusterfs-3.12.2, please open a new bug report.

glusterfs-glusterfs-3.12.2 has been announced on the Gluster mailinglists [1], packages for several distributions should become available in the near future. Keep an eye on the Gluster Users mailinglist [2] and the update infrastructure for your distribution.

[1] http://lists.gluster.org/pipermail/gluster-users/2017-October/032684.html
[2] https://www.gluster.org/pipermail/gluster-users/


Note You need to log in before you can comment on or make changes to this bug.