cfq: improve fsync performance for small files

PATCH In review Corrado Zoccolo 4 participants 8 messages Jul 20, 2010 – Oct 23, 2010 1 message not in the archive

Subsystems (2)

Block layer Journalling layer for block devices (JBD2)

lore

Share & tools

lore

Corrado Zoccolo

author

2010-07-20 20:08 UTC #0 lore raw

PATCH 1/1 cfq: improve fsync performance for small files

Fsync performance for small files achieved by cfq on high-end disks is lower than what deadline can achieve, due to idling introduced between the sync write happening in process context and the journal commit.

Moreover, when competing with a sequential reader, a process writing small files and fsync-ing them is starved.

This patch fixes the two problems by:
- marking journal commits as WRITE_SYNC, so that they get the REQ_NOIDLE flag set,
- force all queues that have REQ_NOIDLE requests to be put in the noidle tree.

Having the queue associated to the fsync-ing process and the one associated
to journal commits in the noidle tree allows:
- switching between them without idling,
- fairness vs. competing idling queues, since they will be serviced only after the noidle tree expires its slice.

Acked-by Vivek Goyal <[email protected]>

Reviewed-by Jeff Moyer <[email protected]>

Tested-by Jeff Moyer <[email protected]>

Signed-off-by Corrado Zoccolo <[email protected]>

---
 block/cfq-iosched.c |   18 ++++--------------
 fs/jbd/commit.c     |    2 +-
 fs/jbd2/commit.c    |    2 +-
 3 files changed, 6 insertions(+), 16 deletions(-)

block/cfq-iosched.c +4 ~~-14~~

index eb4086f..49dada4 100644
@@ -216,7 +216,6 @@ struct cfq_data {
 	enum wl_type_t serving_type;
 	unsigned long workload_expires;
 	struct cfq_group *serving_group;
-	bool noidle_tree_requires_idle;
 
 	/*
 	 * Each priority tree is sorted by next_request position.  These

@@ -2126,7 +2125,6 @@ static void choose_service_tree(struct cfq_data *cfqd, struct cfq_group *cfqg)
 	slice = max_t(unsigned, slice, CFQ_MIN_TT);
 	cfq_log(cfqd, "workload slice:%d", slice);
 	cfqd->workload_expires = jiffies + slice;
-	cfqd->noidle_tree_requires_idle = false;
 }
 
 static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd)

@@ -3108,7 +3106,9 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq,
 	if (cfqq->queued[0] + cfqq->queued[1] >= 4)
 		cfq_mark_cfqq_deep(cfqq);
 
-	if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
+	if (cfqq->next_rq && (cfqq->next_rq->cmd_flags & REQ_NOIDLE))
+		enable_idle = 0;
+	else if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
 	    (!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq)))
 		enable_idle = 0;
 	else if (sample_valid(cic->ttime_samples)) {

@@ -3421,17 +3421,7 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
 			cfq_slice_expired(cfqd, 1);
 		else if (sync && cfqq_empty &&
 			 !cfq_close_cooperator(cfqd, cfqq)) {
-			cfqd->noidle_tree_requires_idle |=
-				!(rq->cmd_flags & REQ_NOIDLE);
-			/*
-			 * Idling is enabled for SYNC_WORKLOAD.
-			 * SYNC_NOIDLE_WORKLOAD idles at the end of the tree
-			 * only if we processed at least one !REQ_NOIDLE request
-			 */
-			if (cfqd->serving_type == SYNC_WORKLOAD
-			    || cfqd->noidle_tree_requires_idle
-			    || cfqq->cfqg->nr_cfqq == 1)
-				cfq_arm_slice_timer(cfqd);
+			cfq_arm_slice_timer(cfqd);
 		}
 	}

fs/jbd/commit.c +1 -1

index 28a9dda..d97a0c6 100644
@@ -317,7 +317,7 @@ void journal_commit_transaction(journal_t *journal)
 	int first_tag = 0;
 	int tag_flag;
 	int i;
-	int write_op = WRITE;
+	int write_op = WRITE_SYNC;
 
 	/*
 	 * First job: lock down the current transaction and wait for

fs/jbd2/commit.c +1 -1

index 75716d3..a078744 100644
@@ -369,7 +369,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	int tag_bytes = journal_tag_bytes(journal);
 	struct buffer_head *cbh = NULL; /* For transactional checksums */
 	__u32 crc32_sum = ~0;
-	int write_op = WRITE;
+	int write_op = WRITE_SYNC;
 
 	/*
 	 * First job: lock down the current transaction and wait for

Signature · 1 line

1.6.2.5

Corrado Zoccolo

author

2010-08-15 16:38 UTC #1 lore raw

[+tytso]
Hi Theodore,
in order to fix a performance problem with fsync in cfq, we need to mark write requests sent by jbd/jbd2 as synchronous, so cfq knows that some process may be waiting on them, and therefore will avoid idling. Is this change reasonable for you? Should I send the change to jbd*/commit.cc in a separated patch through your tree?

Thanks
Corrado

Quoted 110 lines · Corrado Zoccolo

Fsync performance for small files achieved by cfq on high-end disks is lower than what deadline can achieve, due to idling introduced between the sync write happening in process context and the journal commit.
Moreover, when competing with a sequential reader, a process writing small files and fsync-ing them is starved.
This patch fixes the two problems by:
- marking journal commits as WRITE_SYNC, so that they get the REQ_NOIDLE flag set,
- force all queues that have REQ_NOIDLE requests to be put in the noidle tree.
Having the queue associated to the fsync-ing process and the one associated to journal commits in the noidle tree allows:
- switching between them without idling,
- fairness vs. competing idling queues, since they will be serviced only after the noidle tree expires its slice.
Acked-by Vivek Goyal <[email protected]>
Reviewed-by Jeff Moyer <[email protected]>
Tested-by Jeff Moyer <[email protected]>
Signed-off-by Corrado Zoccolo <[email protected]>
---
block/cfq-iosched.c | 18 ++++--------------
fs/jbd/commit.c | 2 +-
fs/jbd2/commit.c | 2 +-
 3 files changed, 6 insertions(+), 16 deletions(-)
block/cfq-iosched.c +3 -3
index eb4086f..49dada4 100644
@@ -216,7 +216,6 @@ struct cfq_data {
        enum wl_type_t serving_type;
        unsigned long workload_expires;
        struct cfq_group *serving_group;
-       bool noidle_tree_requires_idle;
 
        /*
         * Each priority tree is sorted by next_request position.  These
@@ -2126,7 +2125,6 @@ static void choose_service_tree(struct cfq_data *cfqd, struct cfq_group *cfqg)
        slice = max_t(unsigned, slice, CFQ_MIN_TT);
        cfq_log(cfqd, "workload slice:%d", slice);
        cfqd->workload_expires = jiffies + slice;
-       cfqd->noidle_tree_requires_idle = false;
  }
 
  static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd)
@@ -3108,7 +3106,9 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq,
        if (cfqq->queued[0] + cfqq->queued[1] >= 4)
                cfq_mark_cfqq_deep(cfqq);
 
-       if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
+       if (cfqq->next_rq && (cfqq->next_rq->cmd_flags & REQ_NOIDLE))
+               enable_idle = 0;
+       else if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
            (!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq)))
                enable_idle = 0;
        else if (sample_valid(cic->ttime_samples)) {
@@ -3421,17 +3421,7 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
                        cfq_slice_expired(cfqd, 1);
else if (sync && cfqq_empty && !cfq_close_cooperator(cfqd, cfqq)) {
- cfqd->noidle_tree_requires_idle |=
- !(rq->cmd_flags & REQ_NOIDLE);
- /*
- * Idling is enabled for SYNC_WORKLOAD.
- * SYNC_NOIDLE_WORKLOAD idles at the end of the tree
- * only if we processed at least one !REQ_NOIDLE request
- */
- if (cfqd->serving_type == SYNC_WORKLOAD
-                           || cfqd->noidle_tree_requires_idle
-                           || cfqq->cfqg->nr_cfqq == 1)
- cfq_arm_slice_timer(cfqd);
+ cfq_arm_slice_timer(cfqd); }
}
fs/jbd/commit.c +1 -1
index 28a9dda..d97a0c6 100644
@@ -317,7 +317,7 @@ void journal_commit_transaction(journal_t *journal)
        int first_tag = 0;
        int tag_flag;
        int i;
-       int write_op = WRITE;
+       int write_op = WRITE_SYNC;
 
        /*
         * First job: lock down the current transaction and wait for
fs/jbd2/commit.c +0 -0
index 75716d3..a078744 100644
@@ -369,7 +369,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
        int tag_bytes = journal_tag_bytes(journal);
struct buffer_head *cbh = NULL; /* For transactional checksums */ __u32 crc32_sum = ~0;
- int write_op = WRITE;
+ int write_op = WRITE_SYNC;
/*
* First job: lock down the current transaction and wait for --
1.6.2.5

Signature · 10 lines

__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:[email protected]
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda

Corrado Zoccolo

author

2010-09-12 14:40 UTC #2 lore raw

In reply to Corrado Zoccolo

Hi Jens,
any comment on this patch, or the problem we are trying to solve?

Quoted 110 lines · Corrado Zoccolo

Fsync performance for small files achieved by cfq on high-end disks is lower than what deadline can achieve, due to idling introduced between the sync write happening in process context and the journal commit.
Moreover, when competing with a sequential reader, a process writing small files and fsync-ing them is starved.
This patch fixes the two problems by:
- marking journal commits as WRITE_SYNC, so that they get the REQ_NOIDLE flag set,
- force all queues that have REQ_NOIDLE requests to be put in the noidle tree.
Having the queue associated to the fsync-ing process and the one associated to journal commits in the noidle tree allows:
- switching between them without idling,
- fairness vs. competing idling queues, since they will be serviced only after the noidle tree expires its slice.
Acked-by Vivek Goyal <[email protected]>
Reviewed-by Jeff Moyer <[email protected]>
Tested-by Jeff Moyer <[email protected]>
Signed-off-by Corrado Zoccolo <[email protected]>
---
block/cfq-iosched.c | 18 ++++--------------
fs/jbd/commit.c | 2 +-
fs/jbd2/commit.c | 2 +-
 3 files changed, 6 insertions(+), 16 deletions(-)
block/cfq-iosched.c +3 -3
index eb4086f..49dada4 100644
@@ -216,7 +216,6 @@ struct cfq_data {
        enum wl_type_t serving_type;
        unsigned long workload_expires;
        struct cfq_group *serving_group;
-       bool noidle_tree_requires_idle;
 
        /*
         * Each priority tree is sorted by next_request position.  These
@@ -2126,7 +2125,6 @@ static void choose_service_tree(struct cfq_data *cfqd, struct cfq_group *cfqg)
        slice = max_t(unsigned, slice, CFQ_MIN_TT);
        cfq_log(cfqd, "workload slice:%d", slice);
        cfqd->workload_expires = jiffies + slice;
-       cfqd->noidle_tree_requires_idle = false;
  }
 
  static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd)
@@ -3108,7 +3106,9 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq,
        if (cfqq->queued[0] + cfqq->queued[1] >= 4)
                cfq_mark_cfqq_deep(cfqq);
 
-       if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
+       if (cfqq->next_rq && (cfqq->next_rq->cmd_flags & REQ_NOIDLE))
+               enable_idle = 0;
+       else if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
            (!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq)))
                enable_idle = 0;
        else if (sample_valid(cic->ttime_samples)) {
@@ -3421,17 +3421,7 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
                        cfq_slice_expired(cfqd, 1);
else if (sync && cfqq_empty && !cfq_close_cooperator(cfqd, cfqq)) {
- cfqd->noidle_tree_requires_idle |=
- !(rq->cmd_flags & REQ_NOIDLE);
- /*
- * Idling is enabled for SYNC_WORKLOAD.
- * SYNC_NOIDLE_WORKLOAD idles at the end of the tree
- * only if we processed at least one !REQ_NOIDLE request
- */
- if (cfqd->serving_type == SYNC_WORKLOAD
-                           || cfqd->noidle_tree_requires_idle
-                           || cfqq->cfqg->nr_cfqq == 1)
- cfq_arm_slice_timer(cfqd);
+ cfq_arm_slice_timer(cfqd); }
}
fs/jbd/commit.c +1 -1
index 28a9dda..d97a0c6 100644
@@ -317,7 +317,7 @@ void journal_commit_transaction(journal_t *journal)
        int first_tag = 0;
        int tag_flag;
        int i;
-       int write_op = WRITE;
+       int write_op = WRITE_SYNC;
 
        /*
         * First job: lock down the current transaction and wait for
fs/jbd2/commit.c +0 -0
index 75716d3..a078744 100644
@@ -369,7 +369,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
        int tag_bytes = journal_tag_bytes(journal);
struct buffer_head *cbh = NULL; /* For transactional checksums */ __u32 crc32_sum = ~0;
- int write_op = WRITE;
+ int write_op = WRITE_SYNC;
/*
* First job: lock down the current transaction and wait for --
1.6.2.5

Signature · 10 lines

__________________________________________________________________________

dott. Corrado Zoccolo                          mailto:[email protected]
PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average
man. The average man seeks certainty in the eyes of the onlooker and calls
that self-confidence. The warrior seeks impeccability in his own eyes and
calls that humbleness.
                               Tales of Power - C. Castaneda

Jeff Moyer

2010-09-16 14:41 UTC #3 lore raw

Quoted 2 lines · Corrado Zoccolo

Hi Jens,
any comment on this patch, or the problem we are trying to solve?

Bueller? Bueller?

Quoted 112 lines · Corrado Zoccolo

Quoted 110 lines
Fsync performance for small files achieved by cfq on high-end disks is lower than what deadline can achieve, due to idling introduced between the sync write happening in process context and the journal commit.
Moreover, when competing with a sequential reader, a process writing small files and fsync-ing them is starved.
This patch fixes the two problems by:
- marking journal commits as WRITE_SYNC, so that they get the REQ_NOIDLE flag set,
- force all queues that have REQ_NOIDLE requests to be put in the noidle tree.
Having the queue associated to the fsync-ing process and the one associated to journal commits in the noidle tree allows:
- switching between them without idling,
- fairness vs. competing idling queues, since they will be serviced only after the noidle tree expires its slice.
Acked-by Vivek Goyal <[email protected]>
Reviewed-by Jeff Moyer <[email protected]>
Tested-by Jeff Moyer <[email protected]>
Signed-off-by Corrado Zoccolo <[email protected]>
---
block/cfq-iosched.c | 18 ++++--------------
fs/jbd/commit.c | 2 +-
fs/jbd2/commit.c | 2 +-
 3 files changed, 6 insertions(+), 16 deletions(-)
block/cfq-iosched.c +3 -3
index eb4086f..49dada4 100644
@@ -216,7 +216,6 @@ struct cfq_data {
        enum wl_type_t serving_type;
        unsigned long workload_expires;
        struct cfq_group *serving_group;
-       bool noidle_tree_requires_idle;
 
        /*
         * Each priority tree is sorted by next_request position.  These
@@ -2126,7 +2125,6 @@ static void choose_service_tree(struct cfq_data *cfqd, struct cfq_group *cfqg)
        slice = max_t(unsigned, slice, CFQ_MIN_TT);
        cfq_log(cfqd, "workload slice:%d", slice);
        cfqd->workload_expires = jiffies + slice;
-       cfqd->noidle_tree_requires_idle = false;
  }
 
  static struct cfq_group *cfq_get_next_cfqg(struct cfq_data *cfqd)
@@ -3108,7 +3106,9 @@ cfq_update_idle_window(struct cfq_data *cfqd, struct cfq_queue *cfqq,
        if (cfqq->queued[0] + cfqq->queued[1] >= 4)
                cfq_mark_cfqq_deep(cfqq);
 
-       if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
+       if (cfqq->next_rq && (cfqq->next_rq->cmd_flags & REQ_NOIDLE))
+               enable_idle = 0;
+       else if (!atomic_read(&cic->ioc->nr_tasks) || !cfqd->cfq_slice_idle ||
            (!cfq_cfqq_deep(cfqq) && CFQQ_SEEKY(cfqq)))
                enable_idle = 0;
        else if (sample_valid(cic->ttime_samples)) {
@@ -3421,17 +3421,7 @@ static void cfq_completed_request(struct request_queue *q, struct request *rq)
                        cfq_slice_expired(cfqd, 1);
else if (sync && cfqq_empty && !cfq_close_cooperator(cfqd, cfqq)) {
- cfqd->noidle_tree_requires_idle |=
- !(rq->cmd_flags & REQ_NOIDLE);
- /*
- * Idling is enabled for SYNC_WORKLOAD.
- * SYNC_NOIDLE_WORKLOAD idles at the end of the tree
- * only if we processed at least one !REQ_NOIDLE request
- */
- if (cfqd->serving_type == SYNC_WORKLOAD
-                           || cfqd->noidle_tree_requires_idle
-                           || cfqq->cfqg->nr_cfqq == 1)
- cfq_arm_slice_timer(cfqd);
+ cfq_arm_slice_timer(cfqd); }
}
fs/jbd/commit.c +1 -1
index 28a9dda..d97a0c6 100644
@@ -317,7 +317,7 @@ void journal_commit_transaction(journal_t *journal)
        int first_tag = 0;
        int tag_flag;
        int i;
-       int write_op = WRITE;
+       int write_op = WRITE_SYNC;
 
        /*
         * First job: lock down the current transaction and wait for
fs/jbd2/commit.c +0 -0
index 75716d3..a078744 100644
@@ -369,7 +369,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
        int tag_bytes = journal_tag_bytes(journal);
struct buffer_head *cbh = NULL; /* For transactional checksums */ __u32 crc32_sum = ~0;
- int write_op = WRITE;
+ int write_op = WRITE_SYNC;
/*
* First job: lock down the current transaction and wait for --
1.6.2.5

Jens Axboe

maintainer

2010-09-20 13:23 UTC #4 lore raw

In reply to Corrado Zoccolo

Quoted 2 lines · Corrado Zoccolo

Hi Jens,
any comment on this patch, or the problem we are trying to solve?

I have queued it up for .37.

Signature · 1 line

Jens Axboe

Corrado Zoccolo

author

2010-10-23 17:31 UTC #5 lore raw

Hi Jens,
I've not seen this patch in your pull request to Linus for .37. Any problem with it during test?

Thanks
Corrado

Quoted 10 lines · Jens Axboe

Quoted 2 lines · Corrado Zoccolo
Hi Jens,
any comment on this patch, or the problem we are trying to solve?
I have queued it up for .37.
--
Jens Axboe

--
__________________________________________________________________________

dott. Corrado Zoccolo mailto:[email protected] PhD - Department of Computer Science - University of Pisa, Italy
--------------------------------------------------------------------------
The self-confidence of a warrior is not the self-confidence of the average man. The average man seeks certainty in the eyes of the onlooker and calls that self-confidence. The warrior seeks impeccability in his own eyes and calls that humbleness.
Tales of Power - C. Castaneda

Ingo Molnar

2010-10-23 17:54 UTC #6 lore raw

Quoted 3 lines · Corrado Zoccolo

Hi Jens,
I've not seen this patch in your pull request to Linus for .37. Any problem with it during test?

Your improvement seems to be upstream:

749ef9f: cfq: improve fsync performance for small files

Thanks,

Ingo

Jens Axboe

maintainer

2010-10-23 18:39 UTC #8 lore raw

In reply to a message not in the archive

Quoted 3 lines · Corrado Zoccolo

Hi Jens,
I've not seen this patch in your pull request to Linus for .37. Any problem with it during test?

Nope, Linus pulled it yesterday, it's now upstream. It was in the for-2.6.37/core bits.

Signature · 1 line

Jens Axboe