<div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_default" style="font-family:arial,helvetica,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 21, 2019 at 5:43 PM Yaniv Kaul <<a href="mailto:ykaul@redhat.com">ykaul@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div style="font-family:arial,helvetica,sans-serif"><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, Mar 21, 2019 at 5:23 PM Nithya Balachandran <<a href="mailto:nbalacha@redhat.com" target="_blank">nbalacha@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 21 Mar 2019 at 16:16, Atin Mukherjee <<a href="mailto:amukherj@redhat.com" target="_blank">amukherj@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div>All,</div><div><br></div><div>In the last few releases of glusterfs, with stability as a primary theme of the releases, there has been lots of changes done on the code optimization with an expectation that such changes will have gluster to provide better performance. While many of these changes do help, but off late we have started seeing some diverse effects of them, one especially being the calloc to malloc conversions. While I do understand that malloc syscall will eliminate the extra memset bottleneck which calloc bears, but with recent kernels having in-built strong compiler optimizations I am not sure whether that makes any significant difference, but as I mentioned earlier certainly if this isn't done carefully it can potentially introduce lot of bugs and I'm writing this email to share one of such experiences.<br></div><div><br></div><div>Sanju & I were having troubles for last two days to figure out why <a href="https://review.gluster.org/#/c/glusterfs/+/22388/" rel="noopener nofollow noreferrer" target="_blank">https://review.gluster.org/#/c/glusterfs/+/22388/</a>
wasn't working in Sanju's system but it had no problems running the same fix in my gluster
containers. After spending a significant amount of time, what we now
figured out is that a malloc call [1] (which was a calloc earlier) is the culprit here. As you all can see, in this function we allocate txn_id and copy the event->txn_id into it through gf_uuid_copy () . But when we were debugging this step wise through gdb, txn_id wasn't exactly copied with the exact event->txn_id and it had some junk values which made the glusterd_clear_txn_opinfo to be invoked with a wrong txn_id later on resulting the leaks to remain the same which was the original intention of the fix. <br></div><div><br></div><div>This was quite painful to debug and we had to spend some time to figure this out. Considering we have converted many such calls in past, I'd urge that we review all such conversions and see if there're any side effects to it. Otherwise we might end up running into many potential memory related bugs later on. OTOH, going forward I'd request every patch owners/maintainers to pay some special attention to these conversions and see they are really beneficial and error free. IMO, general guideline should be - for bigger buffers, malloc would make better sense but has to be done carefully, for smaller size, we stick to calloc.</div></div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div><br></div><div>What do others think about it?<br></div></div></div></blockquote><div><br></div><div>I believe that replacing calloc with malloc everywhere without adequate testing and review is not safe and am against doing so for the following reasons:</div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif">No patch should get in without adequate testing and thorough review.</div><div style="font-family:arial,helvetica,sans-serif"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><ol><li>Most of these patches have not been tested, especially the error paths.I have seen some that introduced issues in error scenarios with pointers being non-null.<br></li></ol></div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif">You raise an interesting issue. Why are free'd memory pointers are not NULL'ified? Why does FREE() set ptr = (void *)0xeeeeeeee and not NULL?</div><div style="font-family:arial,helvetica,sans-serif">This is a potential cause for failure. A re-occuring FREE(NULL) is harmless. A FREE(0xeeeeeeee) is a bit more problematic.</div><div style="font-family:arial,helvetica,sans-serif"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><ol><li></li><li>As Raghavendra said, the code assumes that certain elements will be initialized to null/zero and changing that can have consequences which are not immediately obvious. I think it might be worthwhile to go through the already merged calloc->malloc patches to check error paths and so on to see if they are safe.<br></li></ol></div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif">Agreed.</div><div style="font-family:arial,helvetica,sans-serif"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><ol><li></li><li>Making such changes to the libglusterfs code while we are currently working to stabilize the product is not a good idea. The patches take time to review and any errors introduced in the core pieces affect all processes and require significant effort to debug. <br></li></ol></div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif">Let me know when we consider the project stable. I'd argue the way to stabilize it is not stop improving it, but improving its testing. From more tests to cover more code via more tests to more static analysis coverage, to ensuring CI is rock-solid (inc. random errors that pop up from time to time). Not accepting patches to master is not the right approach, unless it's time-boxed somehow. If it is, then it means we don't trust our CI enough, btw.</div><div style="font-family:arial,helvetica,sans-serif"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><ol><li></li></ol></div><div>Yaniv, while the example you provided might make sense to change to malloc, a lot of the other changes, in my opinion, do not for the effort required. For performance testing, smallfile might be a useful tool to see if any of the changes make a difference. That said, I am reluctant to take in patches that change core code significantly without being tested or providing proof of benefits.<br></div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif">Smallfile is part of CI? I am happy to see it documented @ <a href="https://docs.gluster.org/en/latest/Administrator%20Guide/Performance%20Testing/#smallfile-distributed-io-benchmark" target="_blank">https://docs.gluster.org/en/latest/Administrator%20Guide/Performance%20Testing/#smallfile-distributed-io-benchmark</a> , so at least one can know how to execute it manually.</div></div></div></div></div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">Following up the above link to the smallfile repo leads to 404 (I'm assuming we don't have a link checker running on our documentation, so it can break from time to time?)<br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">I assume it's <a href="https://github.com/distributed-system-analysis/smallfile">https://github.com/distributed-system-analysis/smallfile</a> ?</div>Y.<div style="font-family:arial,helvetica,sans-serif" class="gmail_default"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div style="font-family:arial,helvetica,sans-serif"></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div></div><div><br></div><div>We need better performance and scalability but that is going to need changes in our algorithms and fop handling and that is what we need to be working on. Such changes, when done right, will provide more benefits than the micro optimizations. I think it unlikely the micro optimizations will provide much benefit but am willing to be proven wrong if you have numbers that show otherwise.</div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif">I call them nano-optimizations. I believe they really shave only that much, microsecond would be great [looking at <a href="https://people.eecs.berkeley.edu/~rcs/research/interactive_latency.html" target="_blank">https://people.eecs.berkeley.edu/~rcs/research/interactive_latency.html</a> , I'm in that ballpark figure.].</div><div style="font-family:arial,helvetica,sans-serif">Unless we got enough cache misses (btw, would be nice to add likely()/unlikely() to our code, to give a hint to the compiler on code paths). Not sure that helps that much as well, of course - same figures, I estimate.<br></div><div style="font-family:arial,helvetica,sans-serif">But I do like the fact they remove the false assumption that everything is NULL and we don't need to worry about it - it means we need to look at our structures (and while at it, align them!) and make sure we know what we initialize.</div><div style="font-family:arial,helvetica,sans-serif"><br></div><div style="font-family:arial,helvetica,sans-serif">But I certainly understand the resistance and will abandon the patches if not welcome.<br></div><div style="font-family:arial,helvetica,sans-serif">Y.</div><div style="font-family:arial,helvetica,sans-serif"><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>Regards,<br></div><div>Nithya</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div></div><br><div>[1] <a href="https://github.com/gluster/glusterfs/blob/master/xlators/mgmt/glusterd/src/glusterd-op-sm.c#L5681" target="_blank">https://github.com/gluster/glusterfs/blob/master/xlators/mgmt/glusterd/src/glusterd-op-sm.c#L5681</a><br></div></div></div>
_______________________________________________<br>
maintainers mailing list<br>
<a href="mailto:maintainers@gluster.org" target="_blank">maintainers@gluster.org</a><br>
<a href="https://lists.gluster.org/mailman/listinfo/maintainers" rel="noreferrer" target="_blank">https://lists.gluster.org/mailman/listinfo/maintainers</a><br>
</blockquote></div></div>
_______________________________________________<br>
Gluster-devel mailing list<br>
<a href="mailto:Gluster-devel@gluster.org" target="_blank">Gluster-devel@gluster.org</a><br>
<a href="https://lists.gluster.org/mailman/listinfo/gluster-devel" rel="noreferrer" target="_blank">https://lists.gluster.org/mailman/listinfo/gluster-devel</a></blockquote></div></div></div></div></div></div>
</blockquote></div></div></div>