2020-11-11 05:19:15

by Pankaj Gupta

Subject: [PATCH 0/3] md: code cleanups

From: Pankaj Gupta <[email protected]>

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

2020-11-11 05:19:35

by Pankaj Gupta

Subject: [PATCH 1/3] md: improve variable names in md_flush_request()

From: Pankaj Gupta <[email protected]>

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

Signed-off-by: Pankaj Gupta <[email protected]>
---
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

2020-11-11 05:21:30

by Pankaj Gupta

Subject: [PATCH 2/3] md: add comments in md_flush_request()

From: Pankaj Gupta <[email protected]>

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 <[email protected]>
---
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

2020-11-11 05:21:49

by Pankaj Gupta

Subject: [PATCH 3/3] md: use current request time as base for ktime comparisons

From: Pankaj Gupta <[email protected]>

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 <[email protected]>
---
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

2020-11-11 06:54:17

by Paul Menzel

Subject: Re: [PATCH 1/3] md: improve variable names in md_flush_request()

Dear Pankaj,


Thank you for the cleanups.

Am 11.11.20 um 06:16 schrieb Pankaj Gupta:
> From: Pankaj Gupta <[email protected]>
>
> This patch improves readability by using better variable names
> in flush request coalescing logic.

Please do not indent the commit message.

> Signed-off-by: Pankaj Gupta <[email protected]>
> ---
> 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. ;-)

> 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 <[email protected]>


Kind regards,

Paul

2020-11-11 07:25:48

by Pankaj Gupta

Subject: Re: [PATCH 1/3] md: improve variable names in md_flush_request()

Hi Paul,

> > This patch improves readability by using better variable names
> > in flush request coalescing logic.
>
> Please do not indent the commit message.

o.k

>
> > Signed-off-by: Pankaj Gupta <[email protected]>
> > ---
> > 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. ;-)
>
> > 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 <[email protected]>

Thanks,
Pankaj
>
>
> Kind regards,
>
> Paul

2020-11-16 17:34:36

by Song Liu

Subject: Re: [PATCH 0/3] md: code cleanups

On Tue, Nov 10, 2020 at 9:17 PM Pankaj Gupta
<[email protected]> wrote:
>
> From: Pankaj Gupta <[email protected]>
>
> 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.