Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582508 > unrolled thread

[PATCH 00/17] md: cleanup on direct access to bvec table

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/17] md: cleanup on direct access to bvec table  Ming Lei <tom.leiming@gmail.com> - 2017-02-16 12:50 +0100
    [PATCH 17/17] md: raid10: avoid direct access to bvec table in handle_reshape_read_error Ming Lei <tom.leiming@gmail.com> - 2017-02-16 12:50 +0100
    [PATCH 16/17] md: raid10: avoid direct access to bvec table in reshape_request Ming Lei <tom.leiming@gmail.com> - 2017-02-16 12:50 +0100
    [PATCH 08/17] md: raid1: simplify r1buf_pool_free() Ming Lei <tom.leiming@gmail.com> - 2017-02-16 12:50 +0100
    [PATCH 09/17] md: raid1/raid10: use bio helper in *_pool_free Ming Lei <tom.leiming@gmail.com> - 2017-02-16 13:00 +0100
    [PATCH 06/17] md: raid1/raid10: borrow .bi_error as pre-allocated page index Ming Lei <tom.leiming@gmail.com> - 2017-02-16 13:00 +0100
    [PATCH 04/17] md: introduce helpers for dealing with fetch/store preallocated pages in bio Ming Lei <tom.leiming@gmail.com> - 2017-02-16 13:00 +0100
    [PATCH 01/17] block: introduce bio_segments_all() Ming Lei <tom.leiming@gmail.com> - 2017-02-16 13:00 +0100
    Re: [PATCH 00/17] md: cleanup on direct access to bvec table Shaohua Li <shli@kernel.org> - 2017-02-16 23:20 +0100
      Re: [PATCH 00/17] md: cleanup on direct access to bvec table Ming Lei <tom.leiming@gmail.com> - 2017-02-17 02:30 +0100
        Re: [PATCH 00/17] md: cleanup on direct access to bvec table Shaohua Li <shli@kernel.org> - 2017-02-17 05:20 +0100

#1582508 — [PATCH 00/17] md: cleanup on direct access to bvec table

In MD's resync I/O path, there are lots of direct access to bio's
bvec table. This patchset kills most of them, and the conversion
is quite straightforward.

Once direct access to bvec table in MD is cleaned up, we may make
multipage bvec moving on.

Thanks,
Ming

Ming Lei (17):
  block: introduce bio_segments_all()
  block: introduce bio_remove_last_page()
  md: raid1/raid10: use bio_remove_last_page()
  md: introduce helpers for dealing with fetch/store preallocated pages
    in bio
  md: raid1/raid10: use the introduced helpers
  md: raid1/raid10: borrow .bi_error as pre-allocated page index
  md: raid1/raid10: don't use .bi_vcnt to check if all pages are added
  md: raid1: simplify r1buf_pool_free()
  md: raid1/raid10: use bio helper in *_pool_free
  md: raid1: remove direct access to bvec table in fix_sync_read_error
  md: raid1: use bio helper in process_checks()
  md: raid1: avoid direct access to bvec table in process_checks()
  md: raid1: use bio_segments_all()
  md: raid10: avoid direct access to bvec table in sync_request_write()
  md: raid10: avoid direct access to bvec table in
    fix_recovery_read_error
  md: raid10: avoid direct access to bvec table in reshape_request
  md: raid10: avoid direct access to bvec table in
    handle_reshape_read_error

 block/bio.c         | 23 ++++++++++++++
 drivers/md/md.h     | 21 +++++++++++++
 drivers/md/raid1.c  | 87 +++++++++++++++++++++++++++++++++++++----------------
 drivers/md/raid10.c | 76 ++++++++++++++++++++++++++++++++++------------
 include/linux/bio.h |  8 +++++
 5 files changed, 169 insertions(+), 46 deletions(-)

-- 
2.7.4

[toc] | [next] | [standalone]

#1582509 — [PATCH 17/17] md: raid10: avoid direct access to bvec table in handle_reshape_read_error

The cost is 128bytes(8*16) stack space in kernel thread context, and
just use the bio helper to retrieve pages from bio.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/raid10.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 5016ce8163c3..d9d7c79a3bd4 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4689,7 +4689,15 @@ static int handle_reshape_read_error(struct mddev *mddev,
 	struct r10bio *r10b = &on_stack.r10_bio;
 	int slot = 0;
 	int idx = 0;
-	struct bio_vec *bvec = r10_bio->master_bio->bi_io_vec;
+	struct bio_vec *bvl;
+	struct page *pages[RESYNC_PAGES];
+
+	/*
+	 * This bio is allocated in reshape_request(), and size
+	 * is still RESYNC_PAGES
+	 */
+	bio_for_each_segment_all(bvl, r10_bio->master_bio, idx)
+		pages[idx] = bvl->bv_page;
 
 	r10b->sector = r10_bio->sector;
 	__raid10_find_phys(&conf->prev, r10b);
@@ -4718,7 +4726,7 @@ static int handle_reshape_read_error(struct mddev *mddev,
 			success = sync_page_io(rdev,
 					       addr,
 					       s << 9,
-					       bvec[idx].bv_page,
+					       pages[idx],
 					       REQ_OP_READ, 0, false);
 			rdev_dec_pending(rdev, mddev);
 			rcu_read_lock();
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582511 — [PATCH 16/17] md: raid10: avoid direct access to bvec table in reshape_request

The cost is 128bytes(8*16) stack space in kernel thread context, and
just use the bio helper to retrieve pages from bio.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/raid10.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index c7d2f73565d9..5016ce8163c3 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -4383,6 +4383,8 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
 	struct bio *blist;
 	struct bio *bio, *read_bio;
 	int sectors_done = 0;
+	struct bio_vec *bvl;
+	struct page *pages[RESYNC_PAGES];
 
 	if (sector_nr == 0) {
 		/* If restarting in the middle, skip the initial sectors */
@@ -4546,9 +4548,12 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr,
 
 	/* Now add as many pages as possible to all of these bios. */
 
+	bio_for_each_segment_all(bvl, r10_bio->devs[0].bio, s)
+		pages[s] = bvl->bv_page;
+
 	nr_sectors = 0;
 	for (s = 0 ; s < max_sectors; s += PAGE_SIZE >> 9) {
-		struct page *page = r10_bio->devs[0].bio->bi_io_vec[s/(PAGE_SIZE>>9)].bv_page;
+		struct page *page = pages[s / (PAGE_SIZE >> 9)];
 		int len = (max_sectors - s) << 9;
 		if (len > PAGE_SIZE)
 			len = PAGE_SIZE;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582512 — [PATCH 08/17] md: raid1: simplify r1buf_pool_free()

This patch gets each page's reference of each bio for resync,
then r1buf_pool_free() gets simplified.

The same policy has been taken in raid10's buf pool allocation/free
too.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/raid1.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index 23a3a678a9ed..ebea8615344a 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -143,9 +143,12 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data)
 	/* If not user-requests, copy the page pointers to all bios */
 	if (!test_bit(MD_RECOVERY_REQUESTED, &pi->mddev->recovery)) {
 		for (i=0; i<RESYNC_PAGES ; i++)
-			for (j=1; j<pi->raid_disks; j++)
-				r1_bio->bios[j]->bi_io_vec[i].bv_page =
+			for (j=1; j<pi->raid_disks; j++) {
+				struct page *page =
 					r1_bio->bios[0]->bi_io_vec[i].bv_page;
+				get_page(page);
+				r1_bio->bios[j]->bi_io_vec[i].bv_page = page;
+			}
 	}
 
 	r1_bio->master_bio = NULL;
@@ -170,12 +173,8 @@ static void r1buf_pool_free(void *__r1_bio, void *data)
 	struct r1bio *r1bio = __r1_bio;
 
 	for (i = 0; i < RESYNC_PAGES; i++)
-		for (j = pi->raid_disks; j-- ;) {
-			if (j == 0 ||
-			    r1bio->bios[j]->bi_io_vec[i].bv_page !=
-			    r1bio->bios[0]->bi_io_vec[i].bv_page)
-				safe_put_page(r1bio->bios[j]->bi_io_vec[i].bv_page);
-		}
+		for (j = pi->raid_disks; j-- ;)
+			safe_put_page(r1bio->bios[j]->bi_io_vec[i].bv_page);
 	for (i=0 ; i < pi->raid_disks; i++)
 		bio_put(r1bio->bios[i]);
 
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582513 — [PATCH 09/17] md: raid1/raid10: use bio helper in *_pool_free

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/raid1.c  | 13 ++++++++++---
 drivers/md/raid10.c | 11 +++++++----
 2 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index ebea8615344a..1dd6b2760fba 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -172,9 +172,16 @@ static void r1buf_pool_free(void *__r1_bio, void *data)
 	int i,j;
 	struct r1bio *r1bio = __r1_bio;
 
-	for (i = 0; i < RESYNC_PAGES; i++)
-		for (j = pi->raid_disks; j-- ;)
-			safe_put_page(r1bio->bios[j]->bi_io_vec[i].bv_page);
+	for (i = 0; i < pi->raid_disks; i++) {
+		struct bio_vec *bvl;
+		struct bio *bio = r1bio->bios[i];
+
+		/* make sure all pages can be freed */
+		bio->bi_vcnt = RESYNC_PAGES;
+
+		bio_for_each_segment_all(bvl, bio, j)
+			safe_put_page(bvl->bv_page);
+	}
 	for (i=0 ; i < pi->raid_disks; i++)
 		bio_put(r1bio->bios[i]);
 
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index 8262f3e1dd93..5c698f3d3083 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -230,10 +230,13 @@ static void r10buf_pool_free(void *__r10_bio, void *data)
 	for (j=0; j < conf->copies; j++) {
 		struct bio *bio = r10bio->devs[j].bio;
 		if (bio) {
-			for (i = 0; i < RESYNC_PAGES; i++) {
-				safe_put_page(bio->bi_io_vec[i].bv_page);
-				bio->bi_io_vec[i].bv_page = NULL;
-			}
+			struct bio_vec *bvl;
+
+			/* make sure all pages can be freed */
+			bio->bi_vcnt = RESYNC_PAGES;
+
+			bio_for_each_segment_all(bvl, bio, i)
+				safe_put_page(bvl->bv_page);
 			bio_put(bio);
 		}
 		bio = r10bio->devs[j].repl_bio;
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582517 — [PATCH 06/17] md: raid1/raid10: borrow .bi_error as pre-allocated page index

Before bio is submitted, it is safe to borrow .bi_error. This
patch uses .bi_error as index of pre-allocated page in bio, so
that we can avoid to mess .bi_vcnt. Especially the old way
will not work any more when multipage bvec is introduced.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/raid1.c  | 12 ++++++++++--
 drivers/md/raid10.c | 14 ++++++++++----
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
index c4791fbd69ac..8904a9149671 100644
--- a/drivers/md/raid1.c
+++ b/drivers/md/raid1.c
@@ -2811,13 +2811,14 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
 				len = sync_blocks<<9;
 		}
 
+		/* borrow .bi_error as pre-allocated page index */
 		for (i = 0 ; i < conf->raid_disks * 2; i++) {
 			bio = r1_bio->bios[i];
 			if (bio->bi_end_io) {
-				page = mdev_get_page_from_bio(bio, bio->bi_vcnt);
+				page = mdev_get_page_from_bio(bio, bio->bi_error++);
 				if (bio_add_page(bio, page, len, 0) == 0) {
 					/* stop here */
-					mdev_put_page_to_bio(bio, bio->bi_vcnt, page);
+					mdev_put_page_to_bio(bio, --bio->bi_error, page);
 					while (i > 0) {
 						i--;
 						bio = r1_bio->bios[i];
@@ -2836,6 +2837,13 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr,
 		sync_blocks -= (len>>9);
 	} while (r1_bio->bios[disk]->bi_vcnt < RESYNC_PAGES);
  bio_full:
+	/* return .bi_error back to bio */
+	for (i = 0 ; i < conf->raid_disks * 2; i++) {
+		bio = r1_bio->bios[i];
+		if (bio->bi_end_io)
+			bio->bi_error = 0;
+	}
+
 	r1_bio->sectors = nr_sectors;
 
 	if (mddev_is_clustered(mddev) &&
diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
index b7dfbca869a3..9cfc22cd1330 100644
--- a/drivers/md/raid10.c
+++ b/drivers/md/raid10.c
@@ -3348,7 +3348,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 
 			bio = r10_bio->devs[i].bio;
 			bio_reset(bio);
-			bio->bi_error = -EIO;
 			rcu_read_lock();
 			rdev = rcu_dereference(conf->mirrors[d].rdev);
 			if (rdev == NULL || test_bit(Faulty, &rdev->flags)) {
@@ -3392,7 +3391,6 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			/* Need to set up for writing to the replacement */
 			bio = r10_bio->devs[i].repl_bio;
 			bio_reset(bio);
-			bio->bi_error = -EIO;
 
 			sector = r10_bio->devs[i].addr;
 			bio->bi_next = biolist;
@@ -3435,14 +3433,15 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 			len = (max_sector - sector_nr) << 9;
 		if (len == 0)
 			break;
+		/* borrow .bi_error as pre-allocated page index */
 		for (bio= biolist ; bio ; bio=bio->bi_next) {
 			struct bio *bio2;
-			page = mdev_get_page_from_bio(bio, bio->bi_vcnt);
+			page = mdev_get_page_from_bio(bio, bio->bi_error++);
 			if (bio_add_page(bio, page, len, 0))
 				continue;
 
 			/* stop here */
-			mdev_put_page_to_bio(bio, bio->bi_vcnt, page);
+			mdev_put_page_to_bio(bio, --bio->bi_error, page);
 			for (bio2 = biolist;
 			     bio2 && bio2 != bio;
 			     bio2 = bio2->bi_next) {
@@ -3456,6 +3455,13 @@ static sector_t raid10_sync_request(struct mddev *mddev, sector_t sector_nr,
 		sector_nr += len>>9;
 	} while (biolist->bi_vcnt < RESYNC_PAGES);
  bio_full:
+	/* return .bi_error back to bio, and set resync's as -EIO */
+	for (bio= biolist ; bio ; bio=bio->bi_next)
+		if (test_bit(MD_RECOVERY_SYNC, &mddev->recovery))
+			bio->bi_error = -EIO;
+		else
+			bio->bi_error = 0;
+
 	r10_bio->sectors = nr_sectors;
 
 	while (biolist) {
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582518 — [PATCH 04/17] md: introduce helpers for dealing with fetch/store preallocated pages in bio

Both raid1 and raid10 uses bio's bvec table to store pre-allocated
pages, then fetch and add it to bio.

This patch introduces two helpers for dealing with the special case,
like what bio_iov_iter_get_pages() does.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/md/md.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/md/md.h b/drivers/md/md.h
index a86ad62079de..21897cb514af 100644
--- a/drivers/md/md.h
+++ b/drivers/md/md.h
@@ -708,4 +708,25 @@ static inline void mddev_clear_unsupported_flags(struct mddev *mddev,
 {
 	mddev->flags &= ~unsupported_flags;
 }
+
+/*
+ * Both raid1 and raid10 use bio's bvec table to store the preallocated
+ * pages, so we introduces the following helpers for this kind of usage.
+ *
+ * Actually the usage is a bit similar with bio_iov_iter_get_pages().
+ *
+ * Please make sure .bi_io_vec[idx] points to one unused vector by the
+ * bio.
+ */
+static inline struct page *mdev_get_page_from_bio(struct bio *bio, unsigned idx)
+{
+	return bio->bi_io_vec[idx].bv_page;
+}
+
+static inline void mdev_put_page_to_bio(struct bio *bio, unsigned idx,
+					struct page *page)
+{
+	bio->bi_io_vec[idx].bv_page = page;
+}
+
 #endif /* _MD_MD_H */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582521 — [PATCH 01/17] block: introduce bio_segments_all()

So that we can replace the direct access to .bi_vcnt.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 include/linux/bio.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/bio.h b/include/linux/bio.h
index 8e521194f6fc..3364b3ed90e7 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -293,6 +293,13 @@ static inline void bio_get_last_bvec(struct bio *bio, struct bio_vec *bv)
 		bv->bv_len = iter.bi_bvec_done;
 }
 
+static inline unsigned bio_segments_all(struct bio *bio)
+{
+	WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED));
+
+	return bio->bi_vcnt;
+}
+
 enum bip_flags {
 	BIP_BLOCK_INTEGRITY	= 1 << 0, /* block layer owns integrity data */
 	BIP_MAPPED_INTEGRITY	= 1 << 1, /* ref tag has been remapped */
-- 
2.7.4

[toc] | [prev] | [next] | [standalone]

#1582972

On Thu, Feb 16, 2017 at 07:45:30PM +0800, Ming Lei wrote:
> In MD's resync I/O path, there are lots of direct access to bio's
> bvec table. This patchset kills most of them, and the conversion
> is quite straightforward.

I don't like this approach. The MD uses a hacky way to manage pages allocated,
this is the root of the problem. The patches add another hack way to do the
management. I'd like to see explict management of the pages, for example, add
data structure in r1bio to manage the pages, then we can use existing API for
all the stuffes we need.

Thanks,
Shaohua

[toc] | [prev] | [next] | [standalone]

#1583017

Hi Shaohua,

On Fri, Feb 17, 2017 at 6:16 AM, Shaohua Li <shli@kernel.org> wrote:
> On Thu, Feb 16, 2017 at 07:45:30PM +0800, Ming Lei wrote:
>> In MD's resync I/O path, there are lots of direct access to bio's
>> bvec table. This patchset kills most of them, and the conversion
>> is quite straightforward.
>
> I don't like this approach. The MD uses a hacky way to manage pages allocated,
> this is the root of the problem. The patches add another hack way to do the

Yes, I agree, and bio_iov_iter_get_pages() uses this kind of hacky way too
actually.

> management. I'd like to see explict management of the pages, for example, add
> data structure in r1bio to manage the pages, then we can use existing API for
> all the stuffes we need.

Yeah, that is definitely clean, but we have to pay the following cost:

- allocate at least N * (128 + 4) bytes per each r1_bio/r10_bio
- N is pool_info.raid_disks for raid1, and conf->copies for raid10

If we are happy to introduce the cost, I can take this way in V1.


Thanks,
Ming Lei

[toc] | [prev] | [next] | [standalone]

#1583078

On Fri, Feb 17, 2017 at 09:25:27AM +0800, Ming Lei wrote:
> Hi Shaohua,
> 
> On Fri, Feb 17, 2017 at 6:16 AM, Shaohua Li <shli@kernel.org> wrote:
> > On Thu, Feb 16, 2017 at 07:45:30PM +0800, Ming Lei wrote:
> >> In MD's resync I/O path, there are lots of direct access to bio's
> >> bvec table. This patchset kills most of them, and the conversion
> >> is quite straightforward.
> >
> > I don't like this approach. The MD uses a hacky way to manage pages allocated,
> > this is the root of the problem. The patches add another hack way to do the
> 
> Yes, I agree, and bio_iov_iter_get_pages() uses this kind of hacky way too
> actually.
> 
> > management. I'd like to see explict management of the pages, for example, add
> > data structure in r1bio to manage the pages, then we can use existing API for
> > all the stuffes we need.
> 
> Yeah, that is definitely clean, but we have to pay the following cost:
> 
> - allocate at least N * (128 + 4) bytes per each r1_bio/r10_bio
> - N is pool_info.raid_disks for raid1, and conf->copies for raid10
> 
> If we are happy to introduce the cost, I can take this way in V1.

It's not a big deal. The inflight bio shouldn't be big, so the r1_bio count
isn't big. We don't waste much.

Thanks,
Shaohua

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web