[llvmlinux] Patch review: vlais-md-raid10.patch

Vinícius Tinti viniciustinti at gmail.com
Sat Jan 31 00:20:37 UTC 2015


Hi Behan,

I have reviewed the original patch for vlais-md-raid10.patch and for me
it is fine. I have just changed the syntax of 'sizeof' to match with all
others that are used in the file. This is done in the first patch.

But I think there is a much simpler way to do this. The old patch was
using kmalloc which can be replaced by dynamic size array variables. I
mean I can create 'u8 foo[mystruct_size]' and then assign this buffer to
'mystruct = (mystruct *) foo'. This will avoid the malloc and place the
buffer in the stack the same way the VLAIS was doing. I think it is
better, what do you think?

First patch:

>From fdb434d27c7cfb6ec5ad84e6b166bce8d96f8dbd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Vin=C3=ADcius=20Tinti?= <viniciustinti at gmail.com>
Date: Fri, 30 Jan 2015 21:28:42 -0200
Subject: [PATCH 1/2] patch: Review vlais-md-raid10.patch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Use the same syntax for 'sizeof' in both types.

Signed-off-by: Vinícius Tinti <viniciustinti at gmail.com>
---
 arch/all/patches/vlais-md-raid10.patch | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arch/all/patches/vlais-md-raid10.patch b/arch/all/patches/vlais-md-raid10.patch
index 53d5dda..e94e784 100644
--- a/arch/all/patches/vlais-md-raid10.patch
+++ b/arch/all/patches/vlais-md-raid10.patch
@@ -4,6 +4,7 @@ Date: Tue, 23 Sep 2014 22:43:17 -0700
 Subject: [PATCH] md, raid10, LLVMLinux: Remove VLAIS from raid10 driver
 
 Signed-off-by: Behan Webster <behanw at converseincode.com>
+Reviewed-by: Vinícius Tinti <viniciustinti at gmail.com>
 Suggested-by: Arnd Bergmann <arnd at arndb.de>
 Cc: Arnd Bergmann <arnd at arndb.de>
 ---
@@ -31,7 +32,7 @@ index 6703751..9c65a9d 100644
  				return biovec->bv_len;
  			return 0;
  		}
-+		r10_bio = kmalloc(sizeof *r10_bio +
++		r10_bio = kmalloc(sizeof(*r10_bio) +
 +				  conf->copies * sizeof(struct r10dev), GFP_NOIO);
 +		if (!r10_bio)
 +			return 0;
@@ -61,7 +62,7 @@ index 6703751..9c65a9d 100644
  	int idx = 0;
  	struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
  
-+	r10b = kmalloc(sizeof *r10b + conf->copies * sizeof(struct r10dev),
++	r10b = kmalloc(sizeof(*r10b) + conf->copies * sizeof(struct r10dev),
 +		       GFP_NOIO);
 +	if (!r10b)
 +		return -ENOMEM;
-- 
1.9.1

Second patch with stack allocations:

>From 8ee0a54814a4c67cc496fe495c5aaa78bd9ae4dd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Vin=C3=ADcius=20Tinti?= <viniciustinti at gmail.com>
Date: Fri, 30 Jan 2015 21:52:12 -0200
Subject: [PATCH 2/2] patch: Improve vlais-md-raid10.patch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This simplifies the patch by avoiding doing memory allocations and
placing the necessary data on stack the same way that the original code
with VLAIS was doing.

Signed-off-by: Vinícius Tinti <viniciustinti at gmail.com>
---
 arch/all/patches/vlais-md-raid10.patch | 64 +++++++---------------------------
 1 file changed, 12 insertions(+), 52 deletions(-)

diff --git a/arch/all/patches/vlais-md-raid10.patch b/arch/all/patches/vlais-md-raid10.patch
index e94e784..f3fa482 100644
--- a/arch/all/patches/vlais-md-raid10.patch
+++ b/arch/all/patches/vlais-md-raid10.patch
@@ -4,18 +4,18 @@ Date: Tue, 23 Sep 2014 22:43:17 -0700
 Subject: [PATCH] md, raid10, LLVMLinux: Remove VLAIS from raid10 driver
 
 Signed-off-by: Behan Webster <behanw at converseincode.com>
-Reviewed-by: Vinícius Tinti <viniciustinti at gmail.com>
+Signed-off-by: Vinícius Tinti <viniciustinti at gmail.com>
 Suggested-by: Arnd Bergmann <arnd at arndb.de>
 Cc: Arnd Bergmann <arnd at arndb.de>
 ---
- drivers/md/raid10.c | 24 ++++++++++++++----------
- 1 file changed, 14 insertions(+), 10 deletions(-)
+ drivers/md/raid10.c | 17 +++++++----------
+ 1 file changed, 7 insertions(+), 10 deletions(-)
 
 diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
-index 6703751..9c65a9d 100644
+index 32e282f..67d32ae 100644
 --- a/drivers/md/raid10.c
 +++ b/drivers/md/raid10.c
-@@ -713,11 +713,7 @@ static int raid10_mergeable_bvec(struct request_queue *q,
+@@ -712,11 +712,9 @@ static int raid10_mergeable_bvec(struct request_queue *q,
  		max = biovec->bv_len;
  
  	if (mddev->merge_check_needed) {
@@ -24,30 +24,13 @@ index 6703751..9c65a9d 100644
 -			struct r10dev devs[conf->copies];
 -		} on_stack;
 -		struct r10bio *r10_bio = &on_stack.r10_bio;
-+		struct r10bio *r10_bio;
++		/* Allocate space for r10bio on stack */
++		u8 r10bio_on_stack[sizeof(struct r10bio) + conf->copies * sizeof(struct r10dev)];
++		struct r10bio *r10_bio = (struct r10bio*) r10bio_on_stack;
  		int s;
  		if (conf->reshape_progress != MaxSector) {
  			/* Cannot give any guidance during reshape */
-@@ -725,6 +721,10 @@ static int raid10_mergeable_bvec(struct request_queue *q,
- 				return biovec->bv_len;
- 			return 0;
- 		}
-+		r10_bio = kmalloc(sizeof(*r10_bio) +
-+				  conf->copies * sizeof(struct r10dev), GFP_NOIO);
-+		if (!r10_bio)
-+			return 0;
- 		r10_bio->sector = sector;
- 		raid10_find_phys(conf, r10_bio);
- 		rcu_read_lock();
-@@ -757,6 +757,7 @@ static int raid10_mergeable_bvec(struct request_queue *q,
- 			}
- 		}
- 		rcu_read_unlock();
-+		kfree(r10_bio);
- 	}
- 	return max;
- }
-@@ -4582,15 +4583,16 @@ static int handle_reshape_read_error(struct mddev *mddev,
+@@ -4576,11 +4574,10 @@ static int handle_reshape_read_error(struct mddev *mddev,
  	/* Use sync reads to get the blocks from somewhere else */
  	int sectors = r10_bio->sectors;
  	struct r10conf *conf = mddev->private;
@@ -56,33 +39,10 @@ index 6703751..9c65a9d 100644
 -		struct r10dev devs[conf->copies];
 -	} on_stack;
 -	struct r10bio *r10b = &on_stack.r10_bio;
-+	struct r10bio *r10b;
++	/* Allocate space for r10bio on stack */
++	u8 r10bio_on_stack[sizeof(struct r10bio) + conf->copies * sizeof(struct r10dev)];
++	struct r10bio *r10b = (struct r10bio *) r10bio_on_stack;
 +
  	int slot = 0;
  	int idx = 0;
  	struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
- 
-+	r10b = kmalloc(sizeof(*r10b) + conf->copies * sizeof(struct r10dev),
-+		       GFP_NOIO);
-+	if (!r10b)
-+		return -ENOMEM;
- 	r10b->sector = r10_bio->sector;
- 	__raid10_find_phys(&conf->prev, r10b);
- 
-@@ -4630,11 +4632,13 @@ static int handle_reshape_read_error(struct mddev *mddev,
- 			/* couldn't read this block, must give up */
- 			set_bit(MD_RECOVERY_INTR,
- 				&mddev->recovery);
-+			kfree(r10b);
- 			return -EIO;
- 		}
- 		sectors -= s;
- 		idx++;
- 	}
-+	kfree(r10b);
- 	return 0;
- }
- 
--- 
-1.9.1
-
-- 
1.9.1

Regards,
Vinicius



More information about the LLVMLinux mailing list