Re: [PATCH 2/2] raid5: For write performance, remove REQ_SYNC when write was odirect.

[NeilBrown <neilb@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 3456 bytes --]

On Mon, 16 Jul 2012 14:42:54 +0800 majianpeng <majianpeng@gmail.com> wrote:

> On 2012-07-16 13:40 NeilBrown <neilb@suse.de> Wrote:
> >On Mon, 16 Jul 2012 09:31:55 +0800 majianpeng <majianpeng@gmail.com> wrote:
> >
> >> 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.
> 
> > 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



> >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);
> >
> >
> >> 
> >> Signed-off-by: Jianpeng Ma <majianpeng@gmail.com>
> >> ---
> >>  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;
> >
> >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 828 bytes --]