On Mon, 16 Jul 2012 14:42:54 +0800 majianpeng [off-list ref] wrote:

On 2012-07-16 13:40 NeilBrown [off-list ref] Wrote:

quoted

On Mon, 16 Jul 2012 09:31:55 +0800 majianpeng [off-list ref] wrote:

quoted

In commit e9c7469bb4f502dafc092166201bea1ad5fc0fbf:
Tejun Heo introduced "implment REQ_FLUSH/FUA support".
But for direct-write-blocks, it maybe for other purpose which like the
regular file.
And this flag will set STRIPE_PREREAD_ACTIVE which decreaed the change
to full write.

But this patch remove REQ_SYNC only judging the WRITE_ODIRECT,it will
contail regular file.So it maybe not correctly.
How can difference odriect_write between regular file or block file?

Hi,
I think you are saying the when REQ_SYNC is used with O_DIRECT writes it is
having a negative effect on throughput because it allows the stripe to be
processed immediately without waiting for more requests to be added to the
stripe.

Normal 'sync' requests use WRITE_SYNC which includes "REQ_NOIDLE" which means
  /* don't anticipate more IO after this one */
O_DIRECT request use WRITE_ODIRECT which does not include this flag.

Using REQ_NOIDEL to difference odirect and sync.Why not using:
 +	if (bi->bi_rw & WRITE_ODIRECT)
 +		bi->bi_rw &= ~REQ_SYNC;

Because that code is wrong.  WRITE_ODIRECT is not one flag, it is two flags
'or'ed together.  So this code does not do what you expect.

The flag WRITE_ODIRECT is only used in odirect-write.

quoted

So maybe we should simply change raid5 to only set STRIPE_PREREAD_ACTIVE if
REQ_NOIDLE is set on the bio.  I think this would have the same effect as
what you are trying to achieve.

Could you please try that and see if it has the desired effect on
performance?

I tested and the performance is the same.

"The same" as what?  The same are your original patch, or the same as without
any patch?

NeilBrown

quoted

Thanks,
NeilBrown

i.e. something like this:

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index d56d74d..2d72a57 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c

@@ -4178,7 +4178,7 @@ static void make_request(struct mddev *mddev, struct bio * bi)

			finish_wait(&conf->wait_for_overlap, &w);
			set_bit(STRIPE_HANDLE, &sh->state);
			clear_bit(STRIPE_DELAYED, &sh->state);
-			if ((bi->bi_rw & REQ_SYNC) &&
+			if ((bi->bi_rw & REQ_NOIDLE) &&
			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
				atomic_inc(&conf->preread_active_stripes);
			release_stripe_plug(mddev, sh);

quoted

Signed-off-by: Jianpeng Ma <redacted>
---
 drivers/md/raid5.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 04348d7..8d2d4d1 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c

@@ -4010,6 +4010,9 @@ static void make_request(struct mddev *mddev, struct bio * bi)
 	     chunk_aligned_read(mddev,bi))
 		return;
 
+	if (bi->bi_rw & WRITE_ODIRECT)
+		bi->bi_rw &= ~REQ_SYNC;
+
 	logical_sector = bi->bi_sector & ~((sector_t)STRIPE_SECTORS-1);
 	last_sector = bi->bi_sector + (bi->bi_size>>9);
 	bi->bi_next = NULL;