<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><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 12:45 PM Atin Mukherjee <<a href="mailto:amukherj@redhat.com">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></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">- I'm not sure I understand what 'wasn't exactly copied' means? It either copied or did not copy the event->txn_id ? Is event->txn_id not fully populated somehow?<br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">- This is a regression caused by 81cbbfd1d870bea49b8aafe7bebb9e8251190918 which I introduced in August 4th, and we are just now discovering it. This is not good.</div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">Without looking, I assume almost all CALLOC->MALLOC changes are done on positive paths of the code, which means it's not tested well.</div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">This file, while having a low code coverage, seems to be covered[1], so I'm not sure how we are finding this now?</div><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></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><br></div><div>What do others think about it?<br></div></div></div></blockquote><div><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">I think I might have been aggressive with the changes, but I do feel they are important in some areas where it makes sense. For example:</div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">libglusterfs/src/inode.c :</div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"> new->inode_hash = (void *)GF_CALLOC(<b>65536, sizeof(struct list_head)</b>,<br> gf_common_mt_list_head);<br> if (!new->inode_hash)<br> goto out;<br><br> new->name_hash = (void *)GF_CALLOC(<b>new->hashsize, sizeof(struct list_head)</b>,<br> gf_common_mt_list_head);<br> if (!new->name_hash)<br> goto out;<br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">And just few lines later:</div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"> for (i = 0; i < <b>65536</b>; i++) {<br> INIT_LIST_HEAD(&new->inode_hash[i]);<br> }<br><br> for (i = 0; i < <b>new->hashsize</b>; i++) {<br> INIT_LIST_HEAD(&new->name_hash[i]);<br> }<br><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">So this is really a waste of cycles for no good reason. I agree not every CALLOC is worth converting.<br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">One more note, I'd love to be able to measure the effect. But there's no CI job with benchmarks, inc. CPU and memory consumption, which we can evaluate the changes.</div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"><br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">And lastly, we need better performance. We need better scalability. We are not keeping up with HW advancements (especially NVMe, pmem and such) and (just like other storage stacks!) becoming somewhat of a performance bottleneck.<br></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default"></div><div style="font-family:arial,helvetica,sans-serif" class="gmail_default">Y.</div><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></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>
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></div>