obj_bencher: status_printer waits before the first calculation

[dmitryya]

Fix for issue http://tracker.ceph.com/issues/7401
Cause of the issue: ObjBencher::status_printer calculates first step metrics before a bench test run.
Now, ObjBencher::status_printer waits one measurement period before starting calculating metrics.

Note: the ideal mechanism for such situation is: main thread notify the status thread when a test is started -> after that the status thread waits a measurement period and calculates first values. But this is redundantly here.

[loic-bot]

SUCCESS: the output of run-make-check.sh on centos-7 for e353307 is http://paste2.org/te1HtwXc

Sent from GH.

[ghost]

But this is redundantly here.

Could you explain what you mean by that ? I would love to have the ideal solution ;-)

[dmitryya]

If we look to the code, 'status_print' thread is created before we start write/read data to/from rados. So, in 'ideal solution' we have to notify the thread before (at the point when) we started write/read data (after notification the thread waits one measurement period (1sec) to collect statistic) and this point will be just after we create this thread, thus there is no any waiting. Notification mechanism causes complexity without profit in this case.

[dmitryya]

@dachary Does my explanation make sense? What do you think?

[ghost]

I think the correct fix for http://tracker.ceph.com/issues/7401 is

--- a/src/common/obj_bencher.cc
+++ b/src/common/obj_bencher.cc
@@ -108,7 +108,7 @@ void *ObjBencher::status_printer(void *_bencher) {
     else
       bandwidth = 0;
 
-    if (!isnan(bandwidth)) {
+    if (cycleSinceChange && !isnan(bandwidth)) {
       if (bandwidth > data.idata.max_bandwidth)
         data.idata.max_bandwidth = bandwidth;
       if (bandwidth < data.idata.min_bandwidth)

to skip the first cycle because bandwidth neads two measures to be computed.

[dmitryya]

@dachary I have one concern:

• useless and unnecessary line with all zero will be printed (and other useless operation will be made such us avg bandwidth calculation) at the first step:

  
  sec Cur ops   started  finished  avg MB/s  cur MB/s  last lat   avg lat
    0       0         0         0         0         0         -         0
  

  In my point of view, step with all zeros has to be skipped to do not confuse.

[ghost]

@dmitryya it is better to not print the first line, indeed and the rest of the logic can remain the same.

[loic-bot]

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/1214/

• SUCCESS http://jenkins.ceph.dachary.org/job/ceph/OS=centos-7/1214/
• SUCCESS http://jenkins.ceph.dachary.org/job/ceph/OS=ubuntu-14.04/1214/

[ghost]

This will still store zero bandwidth in data history if writing an object takes more than one second (because data.finished - previous_writes will be zero at least once).

[ghost]

Actually the correct patch is

--- a/src/common/obj_bencher.cc
+++ b/src/common/obj_bencher.cc
@@ -108,7 +108,7 @@ void *ObjBencher::status_printer(void *_bencher) {
     else
       bandwidth = 0;
 
-    if (!isnan(bandwidth)) {
+    if (!isnan(bandwidth)  && bandwidth > 0) {
       if (bandwidth > data.idata.max_bandwidth)
         data.idata.max_bandwidth = bandwidth;
       if (bandwidth < data.idata.min_bandwidth)

because we never want to accumulate a bandwidth that is zero. A bandwidth of zero means that the object could not be written in one second (or that it's the first time in the loop). Even if writing an object takes a very long time, the bandwidth cannot be zero, it will just be very small.

[dmitryya]

@dachary Yes, you are absolutely right - we do not need to accumulate a bandwidth that is zero - I missed this moment. Proposed patch looks good, but I still have one concern about I told before: in my opinion, we do not need printing status line when cycleSinceChange is 0 (first line) - because it confuses.
For this reason I proposed moved printing status code into if (cycleSinceChange) { ... } block.
The first loop in status_printing is useless, because bench is not started yet, for this reason, I think, we have to skip it.

There are several approaches for that:

• move code into if (cycleSinceChange) { ... }
• wait measurement period before while(!data.done) { ... } or move waiting from the end loop to beginning - in these cases we can made a small optimisation and remove if (cycleSinceChange) { ... } block as useless (minus one conditional branch which checks every time in loop - it is not so bad)

[ghost]

@dmitryya could you please rebase ?

[loic-bot]

SUCCESS: http://jenkins.ceph.dachary.org/job/ceph/5028/

• SUCCESS http://jenkins.ceph.dachary.org/job/ceph/LABELS=centos-7&&x86_64/5028/
• SUCCESS http://jenkins.ceph.dachary.org/job/ceph/LABELS=ubuntu-14.04&&x86_64/5028/

[ghost]

@dmitryya merged this because it's useful on its own. Would you be so kind as to propose another pull request addressing the points you mentioned in your last comment ?

[dmitryya]

@dachary yes, sure.