Re: [PATCH V4 1/2] RAID1: a new I/O barrier implementation to remove resync window

[NeilBrown <neilb@suse.com>]

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

On Sat, Feb 18 2017, colyli@suse.de wrote:

> @@ -1447,36 +1497,26 @@ static void raid1_write_request(struct mddev *mddev, struct bio *bio,
>  
>  static void raid1_make_request(struct mddev *mddev, struct bio *bio)
>  {
> -	struct r1conf *conf = mddev->private;
> -	struct r1bio *r1_bio;
> +	void (*make_request_fn)(struct mddev *mddev, struct bio *bio);
> +	struct bio *split;
> +	sector_t sectors;
>  
> -	/*
> -	 * make_request() can abort the operation when read-ahead is being
> -	 * used and no empty request is available.
> -	 *
> -	 */
> -	r1_bio = mempool_alloc(conf->r1bio_pool, GFP_NOIO);
> +	make_request_fn = (bio_data_dir(bio) == READ) ?
> +			  raid1_read_request : raid1_write_request;
>  
....
>  
> -	if (bio_data_dir(bio) == READ)
> -		raid1_read_request(mddev, bio, r1_bio);
> -	else
> -		raid1_write_request(mddev, bio, r1_bio);
> +		make_request_fn(mddev, split);
> +	} while (split != bio);
>  }

I don't think the use of make_request_fn() makes the code more readable
or more efficient, and it quite possibly has a cost.
If you left it as

	if (bio_data_dir(bio) == READ)
		raid1_read_request(mddev, bio, r1_bio);
	else
		raid1_write_request(mddev, bio, r1_bio);

then gcc would notice that both raid1_read_request and
raid1_write_request are static functions that are only used once, and
will normally inline them.  This will reduce the total stack depth,
which you expressed some concern about in a previous email.
Using a function pointer like this makes it harder for gcc to perform
that optimization.


NeilBrown

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