[PATCH v3 0/3] zram: add rw_page implementation for zram and clean up unnecessary parameter

[PATCH v3 0/3] zram: add rw_page implementation for zram and clean up unnecessary parameter

[karam.lee]

From: "karam.lee" <karam.lee@lge.com>

Recently rw_page block device operation has been added.
This patchset implements rw_page operation for zram block device
and does some clean-up.

Patches 1~2 are for clean-up.
Patch 3 is for implementation of rw_page operation.
With the rw_page operation, zram can do I/O without allocating a BIO.
It make zram can save time and memory.

karam.lee (3):
  zram: remove bio parameter from zram_bvec_rw().
  zram: change parameter from vaild_io_request()
  zram: implement rw_page operation of zram

 drivers/block/zram/zram_drv.c |   70 +++++++++++++++++++++++++++++++----------
 1 file changed, 54 insertions(+), 16 deletions(-)

-- 
1.7.9.5

[PATCH v3 1/3] zram: remove bio parameter from zram_bvec_rw().

[karam.lee]

From: "karam.lee" <karam.lee@lge.com>

This patch removes an unnecessary parameter(bio)
from zram_bvec_rw() and zram_bvec_read().
zram_bvec_read() doesn't use a bio parameter, so remove it.
zram_bvec_rw() calls a read/write operation not using bio, so a rw parameter
replaces a bio parameter.

Signed-off-by: karam.lee <karam.lee@lge.com>
---
 drivers/block/zram/zram_drv.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 48eccb3..54da18a 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -368,7 +368,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
 }
 
 static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
-			  u32 index, int offset, struct bio *bio)
+			  u32 index, int offset)
 {
 	int ret;
 	struct page *page;
@@ -535,14 +535,13 @@ out:
 }
 
 static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
-			int offset, struct bio *bio)
+			int offset, int rw)
 {
 	int ret;
-	int rw = bio_data_dir(bio);
 
 	if (rw == READ) {
 		atomic64_inc(&zram->stats.num_reads);
-		ret = zram_bvec_read(zram, bvec, index, offset, bio);
+		ret = zram_bvec_read(zram, bvec, index, offset);
 	} else {
 		atomic64_inc(&zram->stats.num_writes);
 		ret = zram_bvec_write(zram, bvec, index, offset);
@@ -718,7 +717,7 @@ out:
 
 static void __zram_make_request(struct zram *zram, struct bio *bio)
 {
-	int offset;
+	int offset, rw;
 	u32 index;
 	struct bio_vec bvec;
 	struct bvec_iter iter;
@@ -733,6 +732,7 @@ static void __zram_make_request(struct zram *zram, struct bio *bio)
 		return;
 	}
 
+	rw = bio_data_dir(bio);
 	bio_for_each_segment(bvec, bio, iter) {
 		int max_transfer_size = PAGE_SIZE - offset;
 
@@ -747,15 +747,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio)
 			bv.bv_len = max_transfer_size;
 			bv.bv_offset = bvec.bv_offset;
 
-			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
+			if (zram_bvec_rw(zram, &bv, index, offset, rw) < 0)
 				goto out;
 
 			bv.bv_len = bvec.bv_len - max_transfer_size;
 			bv.bv_offset += max_transfer_size;
-			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
+			if (zram_bvec_rw(zram, &bv, index + 1, 0, rw) < 0)
 				goto out;
 		} else
-			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
+			if (zram_bvec_rw(zram, &bvec, index, offset, rw) < 0)
 				goto out;
 
 		update_position(&index, &offset, &bvec);
-- 
1.7.9.5

Re: [PATCH v3 1/3] zram: remove bio parameter from zram_bvec_rw().

[Jerome Marchand]

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

On 10/21/2014 09:27 AM, karam.lee@lge.com wrote:
> From: "karam.lee" <karam.lee@lge.com>
> 
> This patch removes an unnecessary parameter(bio)
> from zram_bvec_rw() and zram_bvec_read().
> zram_bvec_read() doesn't use a bio parameter, so remove it.
> zram_bvec_rw() calls a read/write operation not using bio, so a rw parameter
> replaces a bio parameter.
> 
> Signed-off-by: karam.lee <karam.lee@lge.com>

Acked-by: Jerome Marchand <jmarchan@redhat.com>

> ---
>  drivers/block/zram/zram_drv.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 48eccb3..54da18a 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -368,7 +368,7 @@ static int zram_decompress_page(struct zram *zram, char *mem, u32 index)
>  }
>  
>  static int zram_bvec_read(struct zram *zram, struct bio_vec *bvec,
> -			  u32 index, int offset, struct bio *bio)
> +			  u32 index, int offset)
>  {
>  	int ret;
>  	struct page *page;
> @@ -535,14 +535,13 @@ out:
>  }
>  
>  static int zram_bvec_rw(struct zram *zram, struct bio_vec *bvec, u32 index,
> -			int offset, struct bio *bio)
> +			int offset, int rw)
>  {
>  	int ret;
> -	int rw = bio_data_dir(bio);
>  
>  	if (rw == READ) {
>  		atomic64_inc(&zram->stats.num_reads);
> -		ret = zram_bvec_read(zram, bvec, index, offset, bio);
> +		ret = zram_bvec_read(zram, bvec, index, offset);
>  	} else {
>  		atomic64_inc(&zram->stats.num_writes);
>  		ret = zram_bvec_write(zram, bvec, index, offset);
> @@ -718,7 +717,7 @@ out:
>  
>  static void __zram_make_request(struct zram *zram, struct bio *bio)
>  {
> -	int offset;
> +	int offset, rw;
>  	u32 index;
>  	struct bio_vec bvec;
>  	struct bvec_iter iter;
> @@ -733,6 +732,7 @@ static void __zram_make_request(struct zram *zram, struct bio *bio)
>  		return;
>  	}
>  
> +	rw = bio_data_dir(bio);
>  	bio_for_each_segment(bvec, bio, iter) {
>  		int max_transfer_size = PAGE_SIZE - offset;
>  
> @@ -747,15 +747,15 @@ static void __zram_make_request(struct zram *zram, struct bio *bio)
>  			bv.bv_len = max_transfer_size;
>  			bv.bv_offset = bvec.bv_offset;
>  
> -			if (zram_bvec_rw(zram, &bv, index, offset, bio) < 0)
> +			if (zram_bvec_rw(zram, &bv, index, offset, rw) < 0)
>  				goto out;
>  
>  			bv.bv_len = bvec.bv_len - max_transfer_size;
>  			bv.bv_offset += max_transfer_size;
> -			if (zram_bvec_rw(zram, &bv, index + 1, 0, bio) < 0)
> +			if (zram_bvec_rw(zram, &bv, index + 1, 0, rw) < 0)
>  				goto out;
>  		} else
> -			if (zram_bvec_rw(zram, &bvec, index, offset, bio) < 0)
> +			if (zram_bvec_rw(zram, &bvec, index, offset, rw) < 0)
>  				goto out;
>  
>  		update_position(&index, &offset, &bvec);
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v3 2/3] zram: change parameter from vaild_io_request()

[karam.lee]

From: "karam.lee" <karam.lee@lge.com>

This patch changes parameter of valid_io_request for common usage.
The purpose of valid_io_request() is to determine if bio request is
valid or not.
This patch use  I/O start address and size instead of a BIO parameter
for common usage.

Signed-off-by: karam.lee <karam.lee@lge.com>
---
 drivers/block/zram/zram_drv.c |   16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 54da18a..4565fdc 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -206,19 +206,18 @@ static inline int is_partial_io(struct bio_vec *bvec)
 /*
  * Check if request is within bounds and aligned on zram logical blocks.
  */
-static inline int valid_io_request(struct zram *zram, struct bio *bio)
+static inline int valid_io_request(struct zram *zram,
+		sector_t start, unsigned int size)
 {
-	u64 start, end, bound;
+	u64 end, bound;
 
 	/* unaligned request */
-	if (unlikely(bio->bi_iter.bi_sector &
-		     (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
+	if (unlikely(start & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
 		return 0;
-	if (unlikely(bio->bi_iter.bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
+	if (unlikely(size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
 		return 0;
 
-	start = bio->bi_iter.bi_sector;
-	end = start + (bio->bi_iter.bi_size >> SECTOR_SHIFT);
+	end = start + (size >> SECTOR_SHIFT);
 	bound = zram->disksize >> SECTOR_SHIFT;
 	/* out of range range */
 	if (unlikely(start >= bound || end > bound || start > end))
@@ -780,7 +779,8 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
 	if (unlikely(!init_done(zram)))
 		goto error;
 
-	if (!valid_io_request(zram, bio)) {
+	if (!valid_io_request(zram, bio->bi_iter.bi_sector,
+					bio->bi_iter.bi_size)) {
 		atomic64_inc(&zram->stats.invalid_io);
 		goto error;
 	}
-- 
1.7.9.5

Re: [PATCH v3 2/3] zram: change parameter from vaild_io_request()

[Jerome Marchand]

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

On 10/21/2014 09:27 AM, karam.lee@lge.com wrote:
> From: "karam.lee" <karam.lee@lge.com>
> 
> This patch changes parameter of valid_io_request for common usage.
> The purpose of valid_io_request() is to determine if bio request is
> valid or not.
> This patch use  I/O start address and size instead of a BIO parameter
> for common usage.
> 
> Signed-off-by: karam.lee <karam.lee@lge.com>

Acked-by: Jerome Marchand <jmarchan@redhat.com>

> ---
>  drivers/block/zram/zram_drv.c |   16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 54da18a..4565fdc 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -206,19 +206,18 @@ static inline int is_partial_io(struct bio_vec *bvec)
>  /*
>   * Check if request is within bounds and aligned on zram logical blocks.
>   */
> -static inline int valid_io_request(struct zram *zram, struct bio *bio)
> +static inline int valid_io_request(struct zram *zram,
> +		sector_t start, unsigned int size)
>  {
> -	u64 start, end, bound;
> +	u64 end, bound;
>  
>  	/* unaligned request */
> -	if (unlikely(bio->bi_iter.bi_sector &
> -		     (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
> +	if (unlikely(start & (ZRAM_SECTOR_PER_LOGICAL_BLOCK - 1)))
>  		return 0;
> -	if (unlikely(bio->bi_iter.bi_size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
> +	if (unlikely(size & (ZRAM_LOGICAL_BLOCK_SIZE - 1)))
>  		return 0;
>  
> -	start = bio->bi_iter.bi_sector;
> -	end = start + (bio->bi_iter.bi_size >> SECTOR_SHIFT);
> +	end = start + (size >> SECTOR_SHIFT);
>  	bound = zram->disksize >> SECTOR_SHIFT;
>  	/* out of range range */
>  	if (unlikely(start >= bound || end > bound || start > end))
> @@ -780,7 +779,8 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)
>  	if (unlikely(!init_done(zram)))
>  		goto error;
>  
> -	if (!valid_io_request(zram, bio)) {
> +	if (!valid_io_request(zram, bio->bi_iter.bi_sector,
> +					bio->bi_iter.bi_size)) {
>  		atomic64_inc(&zram->stats.invalid_io);
>  		goto error;
>  	}
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v3 3/3] zram: implement rw_page operation of zram

[karam.lee]

From: "karam.lee" <karam.lee@lge.com>

This patch implements rw_page operation for zram block device.

I implemented the feature in zram and tested it.
Test bed was the G2, LG electronic mobile device, whtich has msm8974
processor and 2GB memory.
With a memory allocation test program consuming memory, the system
generates swap.
And operating time of swap_write_page() was measured.

--------------------------------------------------
|             |   operating time   | improvement |
|             |  (20 runs average) |             |
--------------------------------------------------
|with patch   |    1061.15 us      |    +2.4%    |
--------------------------------------------------
|without patch|    1087.35 us      |             |
--------------------------------------------------

Each test(with paged_io,with BIO) result set shows normal distribution
and has equal variance.
I mean the two values are valid result to compare.
I can say operation with paged I/O(without BIO) is faster 2.4% with
confidence level 95%.

Signed-off-by: karam.lee <karam.lee@lge.com>
---
 drivers/block/zram/zram_drv.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 4565fdc..696f0b5 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -810,8 +810,46 @@ static void zram_slot_free_notify(struct block_device *bdev,
 	atomic64_inc(&zram->stats.notify_free);
 }
 
+static int zram_rw_page(struct block_device *bdev, sector_t sector,
+		       struct page *page, int rw)
+{
+	int offset, ret = 1;
+	u32 index;
+	struct zram *zram;
+	struct bio_vec bv;
+
+	zram = bdev->bd_disk->private_data;
+	if (!valid_io_request(zram, sector, PAGE_SIZE)) {
+		atomic64_inc(&zram->stats.invalid_io);
+		ret = -EINVAL;
+		goto out;
+	}
+
+	down_read(&zram->init_lock);
+	if (unlikely(!init_done(zram))) {
+		ret = -ENOMEM;
+		goto out_unlock;
+	}
+
+	index = sector >> SECTORS_PER_PAGE_SHIFT;
+	offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
+
+	bv.bv_page = page;
+	bv.bv_len = PAGE_SIZE;
+	bv.bv_offset = 0;
+
+	ret = zram_bvec_rw(zram, &bv, index, offset, rw);
+
+out_unlock:
+	up_read(&zram->init_lock);
+out:
+	page_endio(page, rw, ret);
+	return ret;
+}
+
 static const struct block_device_operations zram_devops = {
 	.swap_slot_free_notify = zram_slot_free_notify,
+	.rw_page = zram_rw_page,
 	.owner = THIS_MODULE
 };
 
-- 
1.7.9.5

Re: [PATCH v3 3/3] zram: implement rw_page operation of zram

[Jerome Marchand]

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

On 10/21/2014 09:27 AM, karam.lee@lge.com wrote:
> From: "karam.lee" <karam.lee@lge.com>
> 
> This patch implements rw_page operation for zram block device.
> 
> I implemented the feature in zram and tested it.
> Test bed was the G2, LG electronic mobile device, whtich has msm8974
> processor and 2GB memory.
> With a memory allocation test program consuming memory, the system
> generates swap.
> And operating time of swap_write_page() was measured.
> 
> --------------------------------------------------
> |             |   operating time   | improvement |
> |             |  (20 runs average) |             |
> --------------------------------------------------
> |with patch   |    1061.15 us      |    +2.4%    |
> --------------------------------------------------
> |without patch|    1087.35 us      |             |
> --------------------------------------------------
> 
> Each test(with paged_io,with BIO) result set shows normal distribution
> and has equal variance.
> I mean the two values are valid result to compare.
> I can say operation with paged I/O(without BIO) is faster 2.4% with
> confidence level 95%.
> 
> Signed-off-by: karam.lee <karam.lee@lge.com>
> ---
>  drivers/block/zram/zram_drv.c |   38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> index 4565fdc..696f0b5 100644
> --- a/drivers/block/zram/zram_drv.c
> +++ b/drivers/block/zram/zram_drv.c
> @@ -810,8 +810,46 @@ static void zram_slot_free_notify(struct block_device *bdev,
>  	atomic64_inc(&zram->stats.notify_free);
>  }
>  
> +static int zram_rw_page(struct block_device *bdev, sector_t sector,
> +		       struct page *page, int rw)
> +{
> +	int offset, ret = 1;

Small nitpick, but why do you initialize ret to 1? It doesn't seem to be
ever used (nor is 1 a valid return value AFAICT).

It otherwise looks good.

Acked-by: Jerome Marchand <jmarchan@redhat.com>

> +	u32 index;
> +	struct zram *zram;
> +	struct bio_vec bv;
> +
> +	zram = bdev->bd_disk->private_data;
> +	if (!valid_io_request(zram, sector, PAGE_SIZE)) {
> +		atomic64_inc(&zram->stats.invalid_io);
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +
> +	down_read(&zram->init_lock);
> +	if (unlikely(!init_done(zram))) {
> +		ret = -ENOMEM;
> +		goto out_unlock;
> +	}
> +
> +	index = sector >> SECTORS_PER_PAGE_SHIFT;
> +	offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> +
> +	bv.bv_page = page;
> +	bv.bv_len = PAGE_SIZE;
> +	bv.bv_offset = 0;
> +
> +	ret = zram_bvec_rw(zram, &bv, index, offset, rw);
> +
> +out_unlock:
> +	up_read(&zram->init_lock);
> +out:
> +	page_endio(page, rw, ret);
> +	return ret;
> +}
> +
>  static const struct block_device_operations zram_devops = {
>  	.swap_slot_free_notify = zram_slot_free_notify,
> +	.rw_page = zram_rw_page,
>  	.owner = THIS_MODULE
>  };
>  
> 



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v3 3/3] zram: implement rw_page operation of zram

[karam.lee]

On Tue, Oct 21, 2014 at 03:57:29PM +0200, Jerome Marchand wrote:
> On 10/21/2014 09:27 AM, karam.lee@lge.com wrote:
> > From: "karam.lee" <karam.lee@lge.com>
> > 
> > This patch implements rw_page operation for zram block device.
> > 
> > I implemented the feature in zram and tested it.
> > Test bed was the G2, LG electronic mobile device, whtich has msm8974
> > processor and 2GB memory.
> > With a memory allocation test program consuming memory, the system
> > generates swap.
> > And operating time of swap_write_page() was measured.
> > 
> > --------------------------------------------------
> > |             |   operating time   | improvement |
> > |             |  (20 runs average) |             |
> > --------------------------------------------------
> > |with patch   |    1061.15 us      |    +2.4%    |
> > --------------------------------------------------
> > |without patch|    1087.35 us      |             |
> > --------------------------------------------------
> > 
> > Each test(with paged_io,with BIO) result set shows normal distribution
> > and has equal variance.
> > I mean the two values are valid result to compare.
> > I can say operation with paged I/O(without BIO) is faster 2.4% with
> > confidence level 95%.
> > 
> > Signed-off-by: karam.lee <karam.lee@lge.com>
> > ---
> >  drivers/block/zram/zram_drv.c |   38 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 38 insertions(+)
> > 
> > diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
> > index 4565fdc..696f0b5 100644
> > --- a/drivers/block/zram/zram_drv.c
> > +++ b/drivers/block/zram/zram_drv.c
> > @@ -810,8 +810,46 @@ static void zram_slot_free_notify(struct block_device *bdev,
> >  	atomic64_inc(&zram->stats.notify_free);
> >  }
> >  
> > +static int zram_rw_page(struct block_device *bdev, sector_t sector,
> > +		       struct page *page, int rw)
> > +{
> > +	int offset, ret = 1;
> 
> Small nitpick, but why do you initialize ret to 1? It doesn't seem to be
> ever used (nor is 1 a valid return value AFAICT).
> 
> It otherwise looks good.
> 
> Acked-by: Jerome Marchand <jmarchan@redhat.com>
> 

Thank you for reply. I agree with your opinion.

It was my mistake to initialize ret to 1.

I will resend the fixed version.

> > +	u32 index;
> > +	struct zram *zram;
> > +	struct bio_vec bv;
> > +
> > +	zram = bdev->bd_disk->private_data;
> > +	if (!valid_io_request(zram, sector, PAGE_SIZE)) {
> > +		atomic64_inc(&zram->stats.invalid_io);
> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	down_read(&zram->init_lock);
> > +	if (unlikely(!init_done(zram))) {
> > +		ret = -ENOMEM;
> > +		goto out_unlock;
> > +	}
> > +
> > +	index = sector >> SECTORS_PER_PAGE_SHIFT;
> > +	offset = sector & (SECTORS_PER_PAGE - 1) << SECTOR_SHIFT;
> > +
> > +	bv.bv_page = page;
> > +	bv.bv_len = PAGE_SIZE;
> > +	bv.bv_offset = 0;
> > +
> > +	ret = zram_bvec_rw(zram, &bv, index, offset, rw);
> > +
> > +out_unlock:
> > +	up_read(&zram->init_lock);
> > +out:
> > +	page_endio(page, rw, ret);
> > +	return ret;
> > +}
> > +
> >  static const struct block_device_operations zram_devops = {
> >  	.swap_slot_free_notify = zram_slot_free_notify,
> > +	.rw_page = zram_rw_page,
> >  	.owner = THIS_MODULE
> >  };
> >  
> > 
> 
>