<div dir="ltr">The decision of doing away with -O2 is beyond our control, and we shouldn&#39;t have code which depend on optimization to be disabled to behave properly. Representing -errno as return is the cleanest fix (that&#39;s how other projects which use setcontext/getcontext are behaving too.) If there are any new further issues which arise from setcontext/getcontext, I&#39;m tempted to change the internal implementation to use a a vanilla pthread pool.<div>
<br></div><div>Avati</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Dec 11, 2013 at 10:29 PM, Anand Subramanian <span dir="ltr">&lt;<a href="mailto:ansubram@redhat.com" target="_blank">ansubram@redhat.com</a>&gt;</span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Is doing away with -O2 an option that was ever considered or is it that we simply must have O2 on?<br>
<br>
(I understand that turning off O2 can open some so-far-unexposed can of worms and a lot of soaking maybe required, and also that we may have had a good set of perf related reasons to have settled on -O2 in the first place, but wanted to understand nevertheless...)<span class="HOEnZb"><font color="#888888"><br>

<br>
Anand</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
On 12/11/2013 02:21 PM, Pranith Kumar Karampuri wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
hi,<br>
     We found a day-1 bug when syncop_xxx() infra is used inside a synctask with compilation optimization (CFLAGS -O2). This bug has been dormant for at least 2 years.<br>
There are around ~400(rebalance, replace-brick, bd, self-heal-daemon, quota, fuse lock/fd migration) places where syncop is used in the code base all of which are potential candidates which can take the hit.<br>
<br>
I sent first round of patch at <a href="http://review.gluster.com/6475" target="_blank">http://review.gluster.com/6475</a> to catch regressions upstream.<br>
These are the files that are affected by the changes I introduced to fix this:<br>
<br>
  api/src/glfs-fops.c                             |  36 ++++++++++++++++++++++++++++++<u></u>++++<br>
  api/src/glfs-handleops.c                        |  15 ++++++++++++++<br>
  api/src/glfs-internal.h                         |   7 +++++++<br>
  api/src/glfs-resolve.c                          |  10 ++++++++++<br>
  libglusterfs/src/syncop.c                       | 117 ++++++++++++++++++++++++++++++<u></u>++++++++++++++++++++++++++++++<u></u>++++++++++++------------------<u></u>-------------------<br>
  xlators/cluster/afr/src/afr-<u></u>self-heald.c        |  45 +++++++++++++++++++++++++++++-<u></u>------------<br>
  xlators/cluster/afr/src/pump.c                  |  12 ++++++++++--<br>
  xlators/cluster/dht/src/dht-<u></u>helper.c            |  24 +++++++++++++++--------<br>
  xlators/cluster/dht/src/dht-<u></u>rebalance.c         | 168 ++++++++++++++++++++++++++++++<u></u>++++++++++++++++++++++++++++++<u></u>++++++++++++++++++++++++++++++<u></u>++++--------------------------<u></u>------------------------------<u></u>-------<br>

  xlators/cluster/dht/src/dht-<u></u>selfheal.c          |   6 ++++--<br>
  xlators/features/locks/src/<u></u>posix.c              |   3 ++-<br>
  xlators/features/qemu-block/<u></u>src/bdrv-xlator.c   |  15 ++++----------<br>
  xlators/features/qemu-block/<u></u>src/qb-coroutines.c |  14 ++++++++++----<br>
  xlators/mount/fuse/src/fuse-<u></u>bridge.c            |  16 ++++++++++-----<br>
<br>
Please review your respective component for these changes in gerrit.<br>
<br>
Thanks<br>
Pranith.<br>
<br>
Detailed explanation of the Root cause:<br>
We found the bug in &#39;gf_defrag_migrate_data&#39; in rebalance operation:<br>
<br>
Lets look at interesting parts of the function:<br>
<br>
int<br>
gf_defrag_migrate_data (xlator_t *this, gf_defrag_info_t *defrag, loc_t *loc,<br>
                         dict_t *migrate_data)<br>
{<br>
.....<br>
code section - [ Loop ]<br>
         while ((ret = syncop_readdirp (this, fd, 131072, offset, NULL,<br>
                                        &amp;entries)) != 0) {<br>
.....<br>
code section - [ ERRNO-1 ] (errno of readdirp is stored in readdir_operrno by a thread)<br>
                 /* Need to keep track of ENOENT errno, that means, there is no<br>
                    need to send more readdirp() */<br>
                 readdir_operrno = errno;<br>
.....<br>
code section - [ SYNCOP-1 ] (syncop_getxattr is called by a thread)<br>
                         ret = syncop_getxattr (this, &amp;entry_loc, &amp;dict,<br>
                                                GF_XATTR_LINKINFO_KEY);<br>
code section - [ ERRNO-2]   (checking for failures of syncop_getxattr(). This may not always be executed in same thread which executed [SYNCOP-1])<br>
                         if (ret &lt; 0) {<br>
                                 if (errno != ENODATA) {<br>
                                         loglevel = GF_LOG_ERROR;<br>
                                         defrag-&gt;total_failures += 1;<br>
.....<br>
}<br>
<br>
the function above could be executed by thread(t1) till [SYNCOP-1] and code from [ERRNO-2] can be executed by a different thread(t2) because of the way syncop-infra schedules the tasks.<br>
<br>
when the code is compiled with -O2 optimization this is the assembly code that is generated:<br>
  [ERRNO-1]<br>
1165                        readdir_operrno = errno; &lt;&lt;---- errno gets expanded as *(__errno_location())<br>
    0x00007fd149d48b60 &lt;+496&gt;:        callq  0x7fd149d410c0 &lt;__errno_location@plt&gt;<br>
    0x00007fd149d48b72 &lt;+514&gt;:        mov    %rax,0x50(%rsp) &lt;&lt;------ Address returned by __errno_location() is stored in a special location in stack for later use.<br>
    0x00007fd149d48b77 &lt;+519&gt;:        mov    (%rax),%eax<br>
    0x00007fd149d48b79 &lt;+521&gt;:        mov    %eax,0x78(%rsp)<br>
....<br>
  [ERRNO-2]<br>
1281                                        if (errno != ENODATA) {<br>
    0x00007fd149d492ae &lt;+2366&gt;:        mov    0x50(%rsp),%rax &lt;&lt;-----  Because it already stored the address returned by __errno_location(), it just dereferences the address to get the errno value. BUT THIS CODE NEED NOT BE EXECUTED BY SAME THREAD!!!<br>

    0x00007fd149d492b3 &lt;+2371&gt;:        mov    $0x9,%ebp<br>
    0x00007fd149d492b8 &lt;+2376&gt;:        mov    (%rax),%edi<br>
    0x00007fd149d492ba &lt;+2378&gt;:        cmp    $0x3d,%edi<br>
<br>
The problem is that __errno_location() value of t1 and t2 are different. So [ERRNO-2] ends up reading errno of t1 instead of errno of t2 even though t2 is executing [ERRNO-2] code section.<br>
<br>
When code is compiled without any optimization for [ERRNO-2]:<br>
1281                                        if (errno != ENODATA) {<br>
    0x00007fd58e7a326f &lt;+2237&gt;:        callq  0x7fd58e797300 &lt;__errno_location@plt&gt;&lt;&lt;--- As it is calling __errno_location() again it gets the location from t2 so it works as intended.<br>
    0x00007fd58e7a3274 &lt;+2242&gt;:        mov    (%rax),%eax<br>
    0x00007fd58e7a3276 &lt;+2244&gt;:        cmp    $0x3d,%eax<br>
    0x00007fd58e7a3279 &lt;+2247&gt;:        je     0x7fd58e7a32a1 &lt;gf_defrag_migrate_data+2287&gt;<br>
<br>
Fix:<br>
We decided to make syncop_xxx() return (-errno) value as the return value in case of errors and all the functions which make syncop_xxx() will need to use (-ret) to figure out the reason for failure in case of syncop_xxx() failures.<br>

<br>
Pranith<br>
<br>
______________________________<u></u>_________________<br>
Gluster-devel mailing list<br>
<a href="mailto:Gluster-devel@nongnu.org" target="_blank">Gluster-devel@nongnu.org</a><br>
<a href="https://lists.nongnu.org/mailman/listinfo/gluster-devel" target="_blank">https://lists.nongnu.org/<u></u>mailman/listinfo/gluster-devel</a><br>
</blockquote>
<br>
<br>
______________________________<u></u>_________________<br>
Gluster-devel mailing list<br>
<a href="mailto:Gluster-devel@nongnu.org" target="_blank">Gluster-devel@nongnu.org</a><br>
<a href="https://lists.nongnu.org/mailman/listinfo/gluster-devel" target="_blank">https://lists.nongnu.org/<u></u>mailman/listinfo/gluster-devel</a><br>
</div></div></blockquote></div><br></div>