MySQL Bugs: #96519: there is bug when we compute write event slot

Bug #96519	there is bug when we compute write event slot
Submitted:	13 Aug 2019 10:55	Modified:	15 Oct 2019 19:26
Reporter:	Zongzhi Chen (OCA)	Email Updates:
Status:	Closed	Impact on me:	None
Category:	MySQL Server: InnoDB storage engine	Severity:	S3 (Non-critical)
Version:	8.0.*	OS:	Any
Assigned to:		CPU Architecture:	Any

Description:
In the function log_wait_for_write(const log_t &log, lsn_t lsn), when we compute the slot we use

...
  size_t slot =
      (lsn - 1) / OS_FILE_LOG_BLOCK_SIZE & (log.write_events_size - 1);
...

However, in the function compute_write_event_slot, when we compute slot, we don't use lsn - 1, we use lsn directly.

static inline size_t compute_write_event_slot(const log_t &log, lsn_t lsn) {
  return ((lsn / OS_FILE_LOG_BLOCK_SIZE) & (log.write_events_size - 1));
}

so I would suggest to use the same function compute_write_event_slot directly when we compute slot, there is many places that need to get the slot number, such as log_write_notifier, log_flusher_notifier, log_wait_for_write, log_wait_for_flush

Another thing I want to metion is that I don't know why we need lsn -1 when we compute the slot, it may cause useless wake up in this case

when we in the function log_flush_notifier()
void log_flush_notifier(log_t *log_ptr) {
  lsn_t lsn = log.flushed_to_disk_lsn.load() + 1;
  ...
    while (lsn <= notified_up_to_lsn) {
      const auto slot =
          (lsn - 1) / OS_FILE_LOG_BLOCK_SIZE & (log.flush_events_size - 1);

      lsn += OS_FILE_LOG_BLOCK_SIZE;

      LOG_SYNC_POINT("log_flush_notifier_before_notify");

      os_event_set(log.flush_events[slot]);
    }
  ...
}

at the begining if the log.flushed_to_disk_lsn = 511
then lsn_t lsn = 512

in the while loop the first time slot = (512 -1) / OS_FILE_LOG_BLOCK_SIZE  = 0;
the second time slot = (1024 - 1) / OS_FILE_LOG_BLOCK_SIZE = 1;

so we need to wake up slot 0 and slot 1, however, slot 0 must be wake up before, so it is an useless wake up.  We don't need to the lsn minus 1

How to repeat:
Reading the code and perf

Hi Mr. Zongzhi,

Thank you for yoru bug report.

We do , however, need some clarifications .....

First of all, when you write that WE calculate this or that, who are exactly WE ???

If it is some internal source code that you maintain, then why do you report it here ???

Next, what are the effects of this different calculations ??? Can you provide a test case that will show the error ????

Next, let us know the exact release of 8.0 version that you are referring to. Last, but not least, what source code file are we talking about here, which functions and what line numbers ???

Thanks in advance.

Hi Sinisa
I will reorganize the issue

First of all, this is the upstream code of 8.0.17 version, not our own internal source code.

And the code all in innobase/log/log0write.cc

In the function log_wait_for_write(const log_t &log, lsn_t lsn), when Innode compute the slot, Innodbe use this function 

 802   size_t slot =
 803       (lsn - 1) / OS_FILE_LOG_BLOCK_SIZE & (log.write_events_size - 1);
 804
 805   const auto wait_stats =
 806       os_event_wait_for(log.write_events[slot], max_spins,
 807                         srv_log_wait_for_write_timeout, stop_condition);

However, in the function compute_write_event_slot, when InnoDB compute slot, InnoDB don't use lsn - 1, it use lsn directly.

1453 static inline size_t compute_write_event_slot(const log_t &log, lsn_t lsn) {
1454   return ((lsn / OS_FILE_LOG_BLOCK_SIZE) & (log.write_events_size - 1));
1455 }
1456
1457 static inline void notify_about_advanced_write_lsn(log_t &log,
1458                                                    lsn_t old_write_lsn,
1459                                                    lsn_t new_write_lsn) {
1460   if (srv_flush_log_at_trx_commit == 1) {
1461     os_event_set(log.flusher_event);
1462   }
1463
1464   const auto first_slot = compute_write_event_slot(log, old_write_lsn);
1465
1466   const auto last_slot = compute_write_event_slot(log, new_write_lsn);

so I would suggest to use the same function compute_write_event_slot directly when InnoDB compute slot, there is many places that need to get the slot number, such as log_write_notifier, log_flusher_notifier, log_wait_for_write, log_wait_for_flush

Another thing I want to metion is that I don't know why InnoDB need lsn -1 when it compute the slot, it may cause useless wake up in this case

such as:
when we enter into the function log_flush_notifier()

2397 void log_flush_notifier(log_t *log_ptr) {  
2401   lsn_t lsn = log.flushed_to_disk_lsn.load() + 1;
  ...
2462     while (lsn <= notified_up_to_lsn) {
2463       const auto slot =
2464           (lsn - 1) / OS_FILE_LOG_BLOCK_SIZE & (log.flush_events_size - 1);
2465
2466       lsn += OS_FILE_LOG_BLOCK_SIZE;
2467
2468       LOG_SYNC_POINT("log_flush_notifier_before_notify");
2469
2470       os_event_set(log.flush_events[slot]);
2471     }
  ...
}

at the begining.
if the log.flushed_to_disk_lsn = 511
then lsn_t lsn = log.flushed_to_disk_lsn.load() + 1 = 512

in the while loop the first time slot = (512 -1) / OS_FILE_LOG_BLOCK_SIZE  = 0;
the second time slot = (1024 - 1) / OS_FILE_LOG_BLOCK_SIZE = 1;

so InnoDB need to wake up slot 0 and slot 1, however, slot 0 must be wake up before, so it is an useless wake up.  
The root cause is that the lsn don't need to minus 1 when we compute the slot, since lsn stand for the first lsn that havn't been flush before.

At last, to answer your question
The effects of this different calculations will cause useless wakeup

Hi,

I think that the change that you request is small in the implementation, but also small in its effects.

Still, this is a bug.

Verified as reported.

so will you fix it?

Hi,

Since the bug is verified, it will be transferred to our Development, who will determine the scheduling, by themselves ......

Posted by developer:
 
Fixed as of the upcoming 8.0.20 release, and here's the changelog entry:

Various internal functions computed write event slots in an inconsistent
manner.

Thank you, Daniel .....