linux-kernel - Re: [PATCH 4/5] md: implment REQ

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20100824154102.7b68818b@notabene>
Date:	Tue, 24 Aug 2010 15:41:02 +1000
From:	Neil Brown <neilb@...e.de>
To:	Tejun Heo <tj@...nel.org>
Cc:	jaxboe@...ionio.com, linux-fsdevel@...r.kernel.org,
	linux-scsi@...r.kernel.org, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-raid@...r.kernel.org,
	hch@....de, James.Bottomley@...e.de, tytso@....edu,
	chris.mason@...cle.com, swhiteho@...hat.com,
	konishi.ryusuke@....ntt.co.jp, dm-devel@...hat.com, vst@...b.net,
	jack@...e.cz, rwheeler@...hat.com, hare@...e.de,
	rusty@...tcorp.com.au, mst@...hat.com, Tejun Heo <tj@...nle.org>
Subject: Re: [PATCH 4/5] md: implment REQ_FLUSH/FUA support

On Mon, 16 Aug 2010 18:52:02 +0200
Tejun Heo <tj@...nel.org> wrote:


Hi Tejun,
 thanks for doing this.
 It mostly looks good, especially ...


> * REQ_FLUSH/FUA failures are final and its users don't need retry
>   logic.  Retry logic is removed.

This bit - all that retry logic felt so clumsy :-)

Only change I would make is:

>  
> @@ -4083,7 +4089,7 @@ static int make_request(mddev_t *mddev, struct bio * bi)
>  			finish_wait(&conf->wait_for_overlap, &w);
>  			set_bit(STRIPE_HANDLE, &sh->state);
>  			clear_bit(STRIPE_DELAYED, &sh->state);
> -			if (mddev->barrier && 
> +			if (mddev->flush_bio &&
>  			    !test_and_set_bit(STRIPE_PREREAD_ACTIVE, &sh->state))
>  				atomic_inc(&conf->preread_active_stripes);
>  			release_stripe(sh);
> @@ -4106,7 +4112,7 @@ static int make_request(mddev_t *mddev, struct bio * bi)
>  		bio_endio(bi, 0);
>  	}
>  
> -	if (mddev->barrier) {
> +	if (mddev->flush_bio) {
>  		/* We need to wait for the stripes to all be handled.
>  		 * So: wait for preread_active_stripes to drop to 0.
>  		 */

These two in raid5.c aren't quite right.
The first should be changed to test
   bi->bi_rw & REQ_SYNC
rather than
   mddev->flush_bio.
(Assuming the REQ_SYNC means "don't bother waiting for more requests that
might combine with this one to make it all go faster" which I think it does.)

For the second we can just drop the whole if statement.
It was needed so that the all the writes would go done to the underlying
devices so that the null-barrier which would subsequently be passed to all
those devices would go *after* the writes for the barrier request.
As there is no longer a post-flush, that code can go.

Thanks a lot, and sorry for the delay in reviewing it.
NeilBrown


> diff --git a/drivers/md/raid5.h b/drivers/md/raid5.h
> index 0f86f5e..ff9cad2 100644
> --- a/drivers/md/raid5.h
> +++ b/drivers/md/raid5.h
> @@ -275,6 +275,7 @@ struct r6_state {
>  				    * filling
>  				    */
>  #define R5_Wantdrain	13 /* dev->towrite needs to be drained */
> +#define R5_WantFUA	14	/* Write should be FUA */
>  /*
>   * Write method
>   */

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/