Re: [lvm-devel] dm thin: optimize away writing all zeroes to unprovisioned blocks

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hey Jens / Joe / Mike / Marian:

I'm checking in for additional feedback on the patch below (intact in 
previous email of this thread) in the hope of meeting the merge window for 
3.20.

I have implemented the following based on your suggestions:
  * pipelining for performance
  * showed that memcmp against zero page is far slower than 
    directly checking for zero
  * minimized cache effects of nonzero data with early minimal nonzero 
    testing before hitting the pipeline
  * implemented configurability via table reload (pf->skip_zero_writes)
  * implemented status via dmsetup status

Do you have any additional considerations that should be addressed before 
pushing this upstream?

-Eric


On Sun, 25 Jan 2015, Eric Wheeler wrote:

> Introduce bio_is_zero_filled() and use it to optimize away writing all
> zeroes to unprovisioned blocks.  Subsequent reads to the associated
> unprovisioned blocks will be zero filled.  
> 
> Single-thread performance evaluates zero bio's at ~8137MB/s under KVM on a
> Xeon E3-1230.  Zero-checks attempt to minimize cache effects on non-zero
> data. bio_is_zero_filled works with unaligned bvec data. (Using memcmp
> and comparing against the zero page is ~5x slower, so this implementation
> was optimized to increase pipelined exectution.)
> 
> The pool_feature pf.skip_zero_writes is implemented and configurable at
> creation time or via table reload.  
> 
> To test we prepare two dm-thinp volumes test1 and test2 of equal size.
> We format test1 without the patch, mount, and extract the Linux source
> tree onto the test1 filesystem, and unmount.  Finally, we activate
> skip_zero_writes, dd test1 over test2, and verify checksums:
>         b210f032a6465178103317f3c40ab59f  /dev/test/test1
>         b210f032a6465178103317f3c40ab59f  /dev/test/test2
> 
> Notice the allocation difference for thin volumes test1 and test2 (after
> dd if=test1 of=test2), even though they have the same md5sum:
>   LV    VG   Attr       LSize Pool  Origin Data%
>   test1 test Vwi-a-tz-- 4.00g thinp         22.04
>   test2 test Vwi-a-tz-- 4.00g thinp         18.33
> 
> An additional 3.71% of space was saved by the patch, and so were the
> ~150MB of (possibly random) IOs that would have hit disk, not to mention
> reads that now bypass the disk since they are unallocated.  We also save
> the metadata overhead of ~2400 provision_block() allocations.
> 
> 
> Signed-off-by: Eric Wheeler <dm-devel@xxxxxxxxxxxxxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxxxxx>
> Cc: ejt@xxxxxxxxxx
> 
> ---
>  Documentation/device-mapper/thin-provisioning.txt |    2 +
>  block/bio.c                                       |  103 +++++++++++++++++++++
>  drivers/md/dm-thin.c                              |   27 ++++++
>  include/linux/bio.h                               |    1 +
>  4 files changed, 133 insertions(+), 0 deletions(-)
>  http://www.globallinuxsecurity.pro/
> 
> diff --git a/Documentation/device-mapper/thin-provisioning.txt b/Documentation/device-mapper/thin-provisioning.txt
> index 2f51735..7304ccf 100644
> --- a/Documentation/device-mapper/thin-provisioning.txt
> +++ b/Documentation/device-mapper/thin-provisioning.txt
> @@ -266,6 +266,8 @@ i) Constructor
>  
>        error_if_no_space: Error IOs, instead of queueing, if no space.
>  
> +	  enable_zero_writes: Enable all-zero writes to unallocated blocks. 
> +    
>      Data block size must be between 64KB (128 sectors) and 1GB
>      (2097152 sectors) inclusive.
>  
> diff --git a/block/bio.c b/block/bio.c
> index 8c2e55e..7e372b7 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -511,6 +511,109 @@ void zero_fill_bio(struct bio *bio)
>  }
>  EXPORT_SYMBOL(zero_fill_bio);
>  
> +__attribute__((optimize("unroll-loops")))
> +bool bio_is_zero_filled(struct bio *bio)
> +{
> +	struct bio_vec bv;
> +	struct bvec_iter iter;
> +	char *data, *p;
> +	__uint128_t *p128;
> +
> +	unsigned i, count, left;
> +	unsigned long flags;
> +
> +	/* Align to the data chunk size */
> +	const int align_to = sizeof(__uint128_t);
> +
> +	p128 = NULL;
> +	bio_for_each_segment(bv, bio, iter) {
> +		data = bvec_kmap_irq(&bv, &flags);
> +		p = data;
> +		left = bv.bv_len;
> +
> +		if (unlikely( data == NULL ))
> +			continue;
> +
> +		/* check unaligned bytes at the beginning of p, if any */
> +		if (unlikely( ( (uintptr_t)p & (align_to-1) ) != 0 )) {
> +			count = align_to - ( (uintptr_t)p & (align_to-1) );
> +			for (i = 0; i < count; i++) {
> +				if (*p) 
> +					break;
> +				p++;
> +			}
> +			if (i < count)
> +				goto out_false;
> +			left -= count;
> +		}
> +
> +		/* we should be align_to-byte aligned now */
> +		BUG_ON(unlikely( ((uintptr_t)p & (align_to-1) ) != 0 ));
> +
> +		p128 = (__uint128_t*)p;
> +		count = left >> 9; /* count = left / (32*16) */
> +
> +		/* Quickly sample first, middle, and last values 
> +		 * and exit early without hitting the loop if nonzero.
> +		 * This reduces cache effects for non-zero data and has almost
> +		 * no effective penalty since these values will probably be in 
> +		 * cache when we test them below.
> +		 */
> +		if (likely(count) &&
> +			   (p128[0] || p128[left>>5] || p128[(left>>4)-1]))
> +			goto out_false;
> +
> +		/* We are using __uint128_t here, so 32*16=512 bytes per loop 
> +		 * Writing out the 32 operations helps with pipelining. */
> +		for (i = 0; i < count; i++) {
> +			if (p128[ 0] | p128[ 1] | p128[ 2] | p128[ 3] |
> +			    p128[ 4] | p128[ 5] | p128[ 6] | p128[ 7] |
> +			    p128[ 8] | p128[ 9] | p128[10] | p128[11] |
> +			    p128[12] | p128[13] | p128[14] | p128[15] |
> +			    p128[16] | p128[17] | p128[18] | p128[19] |
> +			    p128[20] | p128[21] | p128[22] | p128[23] |
> +			    p128[24] | p128[25] | p128[26] | p128[27] |
> +			    p128[28] | p128[29] | p128[30] | p128[31])
> +				break;
> +
> +			p128 += 32;
> +		}
> +		if (i < count)
> +			goto out_false;
> +
> +		left -= count << 9; /* left -= count * 32*16 */
> +
> +		/* check remaining unaligned values at the end, if any */
> +		if (unlikely(left > 0))	{
> +			p = (char*)p128;
> +			for (i = 0; i < left; i++) {
> +				if (*p)
> +					break;
> +				p++;
> +			}
> +			if (i < left)
> +				goto out_false;
> +
> +			/* Fixup values for the BUG_ON checks below */
> +			p128 = (__uint128_t*)p;
> +			left = 0;
> +		}
> +
> +		bvec_kunmap_irq(data, &flags);
> +		BUG_ON(unlikely( left > 0 ));
> +		BUG_ON(unlikely( data+bv.bv_len != (char*)p128 ));
> +	}
> +
> +	return true;
> +
> +out_false:
> +	if (likely(data != NULL))
> +		bvec_kunmap_irq(data, &flags);
> +
> +	return false;
> +}
> +EXPORT_SYMBOL(bio_is_zero_filled);
> +
>  /**
>   * bio_put - release a reference to a bio
>   * @bio:   bio to release reference to
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index fc9c848..d0cebd2 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -151,6 +151,7 @@ struct pool_features {
>  	bool discard_enabled:1;
>  	bool discard_passdown:1;
>  	bool error_if_no_space:1;
> +	bool skip_zero_writes:1;
>  };
>  
>  struct thin_c;
> @@ -1258,6 +1259,16 @@ static void provision_block(struct thin_c *tc, struct bio *bio, dm_block_t block
>  		return;
>  	}
>  
> +        /*
> +	 *  Skip writes of all zeroes to unprovisioned blocks.
> +	 */
> +
> +	if (tc->pool->pf.skip_zero_writes && bio_is_zero_filled(bio) ) {
> +		cell_defer_no_holder(tc, cell);
> +		bio_endio(bio, 0);
> +		return;
> +	}
> +
>  	r = alloc_data_block(tc, &data_block);
>  	switch (r) {
>  	case 0:
> @@ -2063,6 +2074,7 @@ static void pool_features_init(struct pool_features *pf)
>  	pf->discard_enabled = true;
>  	pf->discard_passdown = true;
>  	pf->error_if_no_space = false;
> +	pf->skip_zero_writes = true;
>  }
>  
>  static void __pool_destroy(struct pool *pool)
> @@ -2310,6 +2322,9 @@ static int parse_pool_features(struct dm_arg_set *as, struct pool_features *pf,
>  		else if (!strcasecmp(arg_name, "error_if_no_space"))
>  			pf->error_if_no_space = true;
>  
> +		else if (!strcasecmp(arg_name, "enable_zero_writes"))
> +			pf->skip_zero_writes = false;
> +
>  		else {
>  			ti->error = "Unrecognised pool feature requested";
>  			r = -EINVAL;
> @@ -2393,6 +2408,7 @@ static dm_block_t calc_metadata_threshold(struct pool_c *pt)
>   *	     no_discard_passdown: don't pass discards down to the data device
>   *	     read_only: Don't allow any changes to be made to the pool metadata.
>   *	     error_if_no_space: error IOs, instead of queueing, if no space.
> + *	     enable_zero_writes: Enable all-zero writes to unallocated blocks.
>   */
>  static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
>  {
> @@ -2511,6 +2527,9 @@ static int pool_ctr(struct dm_target *ti, unsigned argc, char **argv)
>  	}
>  	ti->private = pt;
>  
> +	/* The pf skip_zero_writes is safe to change at any time */
> +	pool->pf.skip_zero_writes = pf.skip_zero_writes;
> +
>  	r = dm_pool_register_metadata_threshold(pt->pool->pmd,
>  						calc_metadata_threshold(pt),
>  						metadata_low_callback,
> @@ -2936,6 +2955,9 @@ static void emit_flags(struct pool_features *pf, char *result,
>  
>  	if (pf->error_if_no_space)
>  		DMEMIT("error_if_no_space ");
> +	
> +	if (!pf->skip_zero_writes)
> +		DMEMIT("enable_zero_writes ");
>  }
>  
>  /*
> @@ -3043,6 +3065,11 @@ static void pool_status(struct dm_target *ti, status_type_t type,
>  		else
>  			DMEMIT("queue_if_no_space ");
>  
> +		if (!pool->pf.skip_zero_writes)
> +			DMEMIT("enable_zero_writes ");
> +		else
> +			DMEMIT("skip_zero_writes ");
> +
>  		break;
>  
>  	case STATUSTYPE_TABLE:
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 5a64576..abb46f7 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -419,6 +419,7 @@ extern struct bio *bio_copy_user_iov(struct request_queue *,
>  				     int, int, gfp_t);
>  extern int bio_uncopy_user(struct bio *);
>  void zero_fill_bio(struct bio *bio);
> +bool bio_is_zero_filled(struct bio *bio);
>  extern struct bio_vec *bvec_alloc(gfp_t, int, unsigned long *, mempool_t *);
>  extern void bvec_free(mempool_t *, struct bio_vec *, unsigned int);
>  extern unsigned int bvec_nr_vecs(unsigned short idx);
> -- 
> 1.7.1
> 
> --
> lvm-devel mailing list
> lvm-devel@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/lvm-devel
> 

--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux