[PATCH 0/3] md: code cleanups

From: Pankaj Gupta <redacted>

This patch series does some cleanups during my attempt to understand
the code. 

Pankaj Gupta (3):
  md: improve variable names in md_flush_request()
  md: add comments in md_flush_request()
  md: use current request time as base for ktime comparisons

 drivers/md/md.c | 12 ++++++++----
 drivers/md/md.h |  6 +++---
 2 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.20.1

From: Pankaj Gupta <redacted>

 Request coalescing logic is dependent on flush time
 update in other context. This patch adds comments
 to understand the code flow better.

Signed-off-by: Pankaj Gupta <redacted>
---
 drivers/md/md.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 167c80f98533..a330e61876e0 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c

@@ -662,10 +662,14 @@ bool md_flush_request(struct mddev *mddev, struct bio *bio)
 {
 	ktime_t req_start = ktime_get_boottime();
 	spin_lock_irq(&mddev->lock);
+	/* flush requests wait until ongoing flush completes,
+	 * hence coalescing all the pending requests.
+	 */
 	wait_event_lock_irq(mddev->sb_wait,
 			    !mddev->flush_bio ||
 			    ktime_after(mddev->prev_flush_start, req_start),
 			    mddev->lock);
+	/* new request after previous flush is completed */
 	if (!ktime_after(mddev->prev_flush_start, req_start)) {
 		WARN_ON(mddev->flush_bio);
 		mddev->flush_bio = bio;

-- 
2.20.1

From: Pankaj Gupta <redacted>

 This patch improves readability by using better variable names
 in flush request coalescing logic.

Signed-off-by: Pankaj Gupta <redacted>
---
 drivers/md/md.c | 8 ++++----
 drivers/md/md.h | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 98bac4f304ae..167c80f98533 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c

@@ -639,7 +639,7 @@ static void md_submit_flush_data(struct work_struct *ws)
 	 * could wait for this and below md_handle_request could wait for those
 	 * bios because of suspend check
 	 */
-	mddev->last_flush = mddev->start_flush;
+	mddev->prev_flush_start = mddev->start_flush;
 	mddev->flush_bio = NULL;
 	wake_up(&mddev->sb_wait);
 

@@ -660,13 +660,13 @@ static void md_submit_flush_data(struct work_struct *ws)
  */
 bool md_flush_request(struct mddev *mddev, struct bio *bio)
 {
-	ktime_t start = ktime_get_boottime();
+	ktime_t req_start = ktime_get_boottime();
 	spin_lock_irq(&mddev->lock);
 	wait_event_lock_irq(mddev->sb_wait,
 			    !mddev->flush_bio ||
-			    ktime_after(mddev->last_flush, start),
+			    ktime_after(mddev->prev_flush_start, req_start),
 			    mddev->lock);
-	if (!ktime_after(mddev->last_flush, start)) {
+	if (!ktime_after(mddev->prev_flush_start, req_start)) {
 		WARN_ON(mddev->flush_bio);
 		mddev->flush_bio = bio;
 		bio = NULL;

diff --git a/drivers/md/md.h b/drivers/md/md.h
index ccfb69868c2e..2292c847f9dd 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h

@@ -495,9 +495,9 @@ struct mddev {
 	 */
 	struct bio *flush_bio;
 	atomic_t flush_pending;
-	ktime_t start_flush, last_flush; /* last_flush is when the last completed
-					  * flush was started.
-					  */
+	ktime_t start_flush, prev_flush_start; /* prev_flush_start is when the previous completed
+						* flush was started.
+						*/
 	struct work_struct flush_work;
 	struct work_struct event_work;	/* used by dm to report failure event */
 	mempool_t *serial_info_pool;

-- 
2.20.1

Dear Pankaj,


Thank you for the cleanups.

Am 11.11.20 um 06:16 schrieb Pankaj Gupta:

From: Pankaj Gupta <redacted>

  This patch improves readability by using better variable names
  in flush request coalescing logic.

Please do not indent the commit message.

With the new variable name, the comment could even be removed. ;-)

  	struct work_struct flush_work;
  	struct work_struct event_work;	/* used by dm to report failure event */
  	mempool_t *serial_info_pool;

Reviewed-by: Paul Menzel <redacted>


Kind regards,

Paul

Hi Paul,

quoted

  This patch improves readability by using better variable names
  in flush request coalescing logic.

Please do not indent the commit message.

o.k

quoted

Signed-off-by: Pankaj Gupta <redacted>
---
  drivers/md/md.c | 8 ++++----
  drivers/md/md.h | 6 +++---
  2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index 98bac4f304ae..167c80f98533 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c

@@ -639,7 +639,7 @@ static void md_submit_flush_data(struct work_struct *ws)
       * could wait for this and below md_handle_request could wait for those
       * bios because of suspend check
       */
-     mddev->last_flush = mddev->start_flush;
+     mddev->prev_flush_start = mddev->start_flush;
      mddev->flush_bio = NULL;
      wake_up(&mddev->sb_wait);

@@ -660,13 +660,13 @@ static void md_submit_flush_data(struct work_struct *ws)
   */
  bool md_flush_request(struct mddev *mddev, struct bio *bio)
  {
-     ktime_t start = ktime_get_boottime();
+     ktime_t req_start = ktime_get_boottime();
      spin_lock_irq(&mddev->lock);
      wait_event_lock_irq(mddev->sb_wait,
                          !mddev->flush_bio ||
-                         ktime_after(mddev->last_flush, start),
+                         ktime_after(mddev->prev_flush_start, req_start),
                          mddev->lock);
-     if (!ktime_after(mddev->last_flush, start)) {
+     if (!ktime_after(mddev->prev_flush_start, req_start)) {
              WARN_ON(mddev->flush_bio);
              mddev->flush_bio = bio;
              bio = NULL;

diff --git a/drivers/md/md.h b/drivers/md/md.h
index ccfb69868c2e..2292c847f9dd 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h

@@ -495,9 +495,9 @@ struct mddev {
       */
      struct bio *flush_bio;
      atomic_t flush_pending;
-     ktime_t start_flush, last_flush; /* last_flush is when the last completed
-                                       * flush was started.
-                                       */
+     ktime_t start_flush, prev_flush_start; /* prev_flush_start is when the previous completed
+                                             * flush was started.
+                                             */

With the new variable name, the comment could even be removed. ;-)

quoted

      struct work_struct flush_work;
      struct work_struct event_work;  /* used by dm to report failure event */
      mempool_t *serial_info_pool;

Reviewed-by: Paul Menzel <redacted>

Thanks,
Pankaj

Kind regards,

Paul

From: Pankaj Gupta <redacted>

Request coalescing logic uses 'prev_flush_start' as base to
compare the current request start time. 'prev_flush_start' is
updated in other context.

This patch changes this by using ktime comparison base to
'req_start' for better readability of code.

Signed-off-by: Pankaj Gupta <redacted>
---
 drivers/md/md.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/md/md.c b/drivers/md/md.c
index a330e61876e0..217b1e3312cb 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c

@@ -667,10 +667,10 @@ bool md_flush_request(struct mddev *mddev, struct bio *bio)
 	 */
 	wait_event_lock_irq(mddev->sb_wait,
 			    !mddev->flush_bio ||
-			    ktime_after(mddev->prev_flush_start, req_start),
+			    ktime_before(req_start, mddev->prev_flush_start),
 			    mddev->lock);
 	/* new request after previous flush is completed */
-	if (!ktime_after(mddev->prev_flush_start, req_start)) {
+	if (ktime_after(req_start, mddev->prev_flush_start)) {
 		WARN_ON(mddev->flush_bio);
 		mddev->flush_bio = bio;
 		bio = NULL;

-- 
2.20.1

On Tue, Nov 10, 2020 at 9:17 PM Pankaj Gupta
[off-list ref] wrote:

From: Pankaj Gupta <redacted>

This patch series does some cleanups during my attempt to understand
the code.

Pankaj Gupta (3):
  md: improve variable names in md_flush_request()
  md: add comments in md_flush_request()
  md: use current request time as base for ktime comparisons

Thanks for the clean up. Applied to md-next.