From: "karam.lee" <[email protected]>
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
From: "karam.lee" <[email protected]>
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 <[email protected]>
---
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
From: "karam.lee" <[email protected]>
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 <[email protected]>
---
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
From: "karam.lee" <[email protected]>
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 <[email protected]>
---
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
On 10/21/2014 09:27 AM, [email protected] wrote:
> From: "karam.lee" <[email protected]>
>
> 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 <[email protected]>
> ---
> 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 <[email protected]>
> + 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
> };
>
>
On 10/21/2014 09:27 AM, [email protected] wrote:
> From: "karam.lee" <[email protected]>
>
> 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 <[email protected]>
Acked-by: Jerome Marchand <[email protected]>
> ---
> 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;
> }
>
On 10/21/2014 09:27 AM, [email protected] wrote:
> From: "karam.lee" <[email protected]>
>
> 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 <[email protected]>
Acked-by: Jerome Marchand <[email protected]>
> ---
> 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);
>
On Tue, Oct 21, 2014 at 03:57:29PM +0200, Jerome Marchand wrote:
> On 10/21/2014 09:27 AM, [email protected] wrote:
> > From: "karam.lee" <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>
>
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
> > };
> >
> >
>
>