| Message ID | 1488296503-4987-7-git-send-email-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 28, 2017 at 11:41:36PM +0800, Ming Lei wrote: > Now we allocate one page array for managing resync pages, instead > of using bio's vec table to do that, and the old way is very hacky > and won't work any more if multipage bvec is enabled. > > The introduced cost is that we need to allocate (128 + 16) * raid_disks > bytes per r1_bio, and it is fine because the inflight r1_bio for > resync shouldn't be much, as pointed by Shaohua. > > Also the bio_reset() in raid1_sync_request() is removed because > all bios are freshly new now and not necessary to reset any more. > > This patch can be thought as a cleanup too > > Suggested-by: Shaohua Li <shli@kernel.org> > Signed-off-by: Ming Lei <tom.leiming@gmail.com> > --- > drivers/md/raid1.c | 83 ++++++++++++++++++++++++++++++++++-------------------- > 1 file changed, 53 insertions(+), 30 deletions(-) > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index c442b4657e2f..900144f39630 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -77,6 +77,16 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr); > #define raid1_log(md, fmt, args...) \ > do { if ((md)->queue) blk_add_trace_msg((md)->queue, "raid1 " fmt, ##args); } while (0) > > +static inline struct resync_pages *get_resync_pages(struct bio *bio) > +{ > + return bio->bi_private; > +} > + > +static inline struct r1bio *get_resync_r1bio(struct bio *bio) > +{ > + return get_resync_pages(bio)->raid_bio; > +} This is a weird between bio, r1bio and the resync_pages. I'd like the pages are embedded in r1bio. Maybe a pointer of r1bio to the pages. It's cleaner and more straightforward. I think the patch 6, 7 8 should be in a same patch. Otherwise bisect will be broken. Thanks, Shaohua
Hi Shaohua, On Wed, Mar 1, 2017 at 7:37 AM, Shaohua Li <shli@kernel.org> wrote: > On Tue, Feb 28, 2017 at 11:41:36PM +0800, Ming Lei wrote: >> Now we allocate one page array for managing resync pages, instead >> of using bio's vec table to do that, and the old way is very hacky >> and won't work any more if multipage bvec is enabled. >> >> The introduced cost is that we need to allocate (128 + 16) * raid_disks >> bytes per r1_bio, and it is fine because the inflight r1_bio for >> resync shouldn't be much, as pointed by Shaohua. >> >> Also the bio_reset() in raid1_sync_request() is removed because >> all bios are freshly new now and not necessary to reset any more. >> >> This patch can be thought as a cleanup too >> >> Suggested-by: Shaohua Li <shli@kernel.org> >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> >> --- >> drivers/md/raid1.c | 83 ++++++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 53 insertions(+), 30 deletions(-) >> >> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c >> index c442b4657e2f..900144f39630 100644 >> --- a/drivers/md/raid1.c >> +++ b/drivers/md/raid1.c >> @@ -77,6 +77,16 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr); >> #define raid1_log(md, fmt, args...) \ >> do { if ((md)->queue) blk_add_trace_msg((md)->queue, "raid1 " fmt, ##args); } while (0) >> >> +static inline struct resync_pages *get_resync_pages(struct bio *bio) >> +{ >> + return bio->bi_private; >> +} >> + >> +static inline struct r1bio *get_resync_r1bio(struct bio *bio) >> +{ >> + return get_resync_pages(bio)->raid_bio; >> +} > > This is a weird between bio, r1bio and the resync_pages. I'd like the pages are It is only a bit weird inside allocating and freeing r1bio, once all are allocated, you can see everthing is clean and simple: - r1bio includes lots of bioes, - and one bio is attached by one resync_pages via .bi_private > embedded in r1bio. Maybe a pointer of r1bio to the pages. It's cleaner and more > straightforward. In theory, the cleanest way is to allocate one resync_pages for each resync bio, but that isn't efficient. That is why this patch allocates all resync_pages together in r1buf_pool_alloc(), and split them into bio. BTW, the only trick is just that the whole page array is stored in .bi_private of the 1st bio, then we can avoid to add one pointer into r1bio. > > I think the patch 6, 7 8 should be in a same patch. Otherwise bisect will be broken. > No, it won't. Both patch 7 and patch 8 just replacing reading from bvec table with reading from the pre-allocated page array. Both should be fine, but the latter is cleaner and simpler to do. Thanks, Ming
On Thu, Mar 02, 2017 at 10:25:10AM +0800, Ming Lei wrote: > Hi Shaohua, > > On Wed, Mar 1, 2017 at 7:37 AM, Shaohua Li <shli@kernel.org> wrote: > > On Tue, Feb 28, 2017 at 11:41:36PM +0800, Ming Lei wrote: > >> Now we allocate one page array for managing resync pages, instead > >> of using bio's vec table to do that, and the old way is very hacky > >> and won't work any more if multipage bvec is enabled. > >> > >> The introduced cost is that we need to allocate (128 + 16) * raid_disks > >> bytes per r1_bio, and it is fine because the inflight r1_bio for > >> resync shouldn't be much, as pointed by Shaohua. > >> > >> Also the bio_reset() in raid1_sync_request() is removed because > >> all bios are freshly new now and not necessary to reset any more. > >> > >> This patch can be thought as a cleanup too > >> > >> Suggested-by: Shaohua Li <shli@kernel.org> > >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> > >> --- > >> drivers/md/raid1.c | 83 ++++++++++++++++++++++++++++++++++-------------------- > >> 1 file changed, 53 insertions(+), 30 deletions(-) > >> > >> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > >> index c442b4657e2f..900144f39630 100644 > >> --- a/drivers/md/raid1.c > >> +++ b/drivers/md/raid1.c > >> @@ -77,6 +77,16 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr); > >> #define raid1_log(md, fmt, args...) \ > >> do { if ((md)->queue) blk_add_trace_msg((md)->queue, "raid1 " fmt, ##args); } while (0) > >> > >> +static inline struct resync_pages *get_resync_pages(struct bio *bio) > >> +{ > >> + return bio->bi_private; > >> +} > >> + > >> +static inline struct r1bio *get_resync_r1bio(struct bio *bio) > >> +{ > >> + return get_resync_pages(bio)->raid_bio; > >> +} > > > > This is a weird between bio, r1bio and the resync_pages. I'd like the pages are > > It is only a bit weird inside allocating and freeing r1bio, once all > are allocated, you > can see everthing is clean and simple: > > - r1bio includes lots of bioes, > - and one bio is attached by one resync_pages via .bi_private I don't how complex to let r1bio pointer to the pages, but that's the nartual way. r1bio owns the pages, not the pages own r1bio, so we should let r1bio points to the pages. The bio.bi_private still points to r1bio. > > embedded in r1bio. Maybe a pointer of r1bio to the pages. It's cleaner and more > > straightforward. > > In theory, the cleanest way is to allocate one resync_pages for each resync bio, > but that isn't efficient. That is why this patch allocates all > resync_pages together > in r1buf_pool_alloc(), and split them into bio. > > BTW, the only trick is just that the whole page array is stored in .bi_private > of the 1st bio, then we can avoid to add one pointer into r1bio. You will need to add pointer in resync_pages which points to r1bio > > > > I think the patch 6, 7 8 should be in a same patch. Otherwise bisect will be broken. > > > > No, it won't. Both patch 7 and patch 8 just replacing reading from bvec table > with reading from the pre-allocated page array. Both should be fine, but the > latter is cleaner and simpler to do. Ok, sounds good, though I doubt it's really worthy. Thanks, Shaohua
On Fri, Mar 3, 2017 at 1:48 AM, Shaohua Li <shli@kernel.org> wrote: > On Thu, Mar 02, 2017 at 10:25:10AM +0800, Ming Lei wrote: >> Hi Shaohua, >> >> On Wed, Mar 1, 2017 at 7:37 AM, Shaohua Li <shli@kernel.org> wrote: >> > On Tue, Feb 28, 2017 at 11:41:36PM +0800, Ming Lei wrote: >> >> Now we allocate one page array for managing resync pages, instead >> >> of using bio's vec table to do that, and the old way is very hacky >> >> and won't work any more if multipage bvec is enabled. >> >> >> >> The introduced cost is that we need to allocate (128 + 16) * raid_disks >> >> bytes per r1_bio, and it is fine because the inflight r1_bio for >> >> resync shouldn't be much, as pointed by Shaohua. >> >> >> >> Also the bio_reset() in raid1_sync_request() is removed because >> >> all bios are freshly new now and not necessary to reset any more. >> >> >> >> This patch can be thought as a cleanup too >> >> >> >> Suggested-by: Shaohua Li <shli@kernel.org> >> >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> >> >> --- >> >> drivers/md/raid1.c | 83 ++++++++++++++++++++++++++++++++++-------------------- >> >> 1 file changed, 53 insertions(+), 30 deletions(-) >> >> >> >> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c >> >> index c442b4657e2f..900144f39630 100644 >> >> --- a/drivers/md/raid1.c >> >> +++ b/drivers/md/raid1.c >> >> @@ -77,6 +77,16 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr); >> >> #define raid1_log(md, fmt, args...) \ >> >> do { if ((md)->queue) blk_add_trace_msg((md)->queue, "raid1 " fmt, ##args); } while (0) >> >> >> >> +static inline struct resync_pages *get_resync_pages(struct bio *bio) >> >> +{ >> >> + return bio->bi_private; >> >> +} >> >> + >> >> +static inline struct r1bio *get_resync_r1bio(struct bio *bio) >> >> +{ >> >> + return get_resync_pages(bio)->raid_bio; >> >> +} >> > >> > This is a weird between bio, r1bio and the resync_pages. I'd like the pages are >> >> It is only a bit weird inside allocating and freeing r1bio, once all >> are allocated, you >> can see everthing is clean and simple: >> >> - r1bio includes lots of bioes, >> - and one bio is attached by one resync_pages via .bi_private > > I don't how complex to let r1bio pointer to the pages, but that's the nartual > way. r1bio owns the pages, not the pages own r1bio, so we should let r1bio > points to the pages. The bio.bi_private still points to r1bio. Actually it is bio which owns the pages for doing its own I/O, and the only thing related with r10bio is that bios may share these pages, but using page refcount trick will make the relation quite implicit. The only reason to allocate all resync_pages together is for sake of efficiency, and just for avoiding to allocate one resync_pages one time for each bio. We have to make .bi_private point to resync_pages(per bio), otherwise we can't fetch pages into one bio at all, thinking about where to store the index for each bio's pre-allocated pages, and it has to be per bio. > >> > embedded in r1bio. Maybe a pointer of r1bio to the pages. It's cleaner and more >> > straightforward. >> >> In theory, the cleanest way is to allocate one resync_pages for each resync bio, >> but that isn't efficient. That is why this patch allocates all >> resync_pages together >> in r1buf_pool_alloc(), and split them into bio. >> >> BTW, the only trick is just that the whole page array is stored in .bi_private >> of the 1st bio, then we can avoid to add one pointer into r1bio. > > You will need to add pointer in resync_pages which points to r1bio Yeah, that is just what this patchset is doing. > >> > >> > I think the patch 6, 7 8 should be in a same patch. Otherwise bisect will be broken. >> > >> >> No, it won't. Both patch 7 and patch 8 just replacing reading from bvec table >> with reading from the pre-allocated page array. Both should be fine, but the >> latter is cleaner and simpler to do. > > Ok, sounds good, though I doubt it's really worthy. Small patch with one purpose is always easy to review, do one thing, do it better, :-) Thanks, Ming Lei
On Fri, Mar 03, 2017 at 10:11:31AM +0800, Ming Lei wrote: > On Fri, Mar 3, 2017 at 1:48 AM, Shaohua Li <shli@kernel.org> wrote: > > On Thu, Mar 02, 2017 at 10:25:10AM +0800, Ming Lei wrote: > >> Hi Shaohua, > >> > >> On Wed, Mar 1, 2017 at 7:37 AM, Shaohua Li <shli@kernel.org> wrote: > >> > On Tue, Feb 28, 2017 at 11:41:36PM +0800, Ming Lei wrote: > >> >> Now we allocate one page array for managing resync pages, instead > >> >> of using bio's vec table to do that, and the old way is very hacky > >> >> and won't work any more if multipage bvec is enabled. > >> >> > >> >> The introduced cost is that we need to allocate (128 + 16) * raid_disks > >> >> bytes per r1_bio, and it is fine because the inflight r1_bio for > >> >> resync shouldn't be much, as pointed by Shaohua. > >> >> > >> >> Also the bio_reset() in raid1_sync_request() is removed because > >> >> all bios are freshly new now and not necessary to reset any more. > >> >> > >> >> This patch can be thought as a cleanup too > >> >> > >> >> Suggested-by: Shaohua Li <shli@kernel.org> > >> >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> > >> >> --- > >> >> drivers/md/raid1.c | 83 ++++++++++++++++++++++++++++++++++-------------------- > >> >> 1 file changed, 53 insertions(+), 30 deletions(-) > >> >> > >> >> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > >> >> index c442b4657e2f..900144f39630 100644 > >> >> --- a/drivers/md/raid1.c > >> >> +++ b/drivers/md/raid1.c > >> >> @@ -77,6 +77,16 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr); > >> >> #define raid1_log(md, fmt, args...) \ > >> >> do { if ((md)->queue) blk_add_trace_msg((md)->queue, "raid1 " fmt, ##args); } while (0) > >> >> > >> >> +static inline struct resync_pages *get_resync_pages(struct bio *bio) > >> >> +{ > >> >> + return bio->bi_private; > >> >> +} > >> >> + > >> >> +static inline struct r1bio *get_resync_r1bio(struct bio *bio) > >> >> +{ > >> >> + return get_resync_pages(bio)->raid_bio; > >> >> +} > >> > > >> > This is a weird between bio, r1bio and the resync_pages. I'd like the pages are > >> > >> It is only a bit weird inside allocating and freeing r1bio, once all > >> are allocated, you > >> can see everthing is clean and simple: > >> > >> - r1bio includes lots of bioes, > >> - and one bio is attached by one resync_pages via .bi_private > > > > I don't how complex to let r1bio pointer to the pages, but that's the nartual > > way. r1bio owns the pages, not the pages own r1bio, so we should let r1bio > > points to the pages. The bio.bi_private still points to r1bio. > > Actually it is bio which owns the pages for doing its own I/O, and the only > thing related with r10bio is that bios may share these pages, but using > page refcount trick will make the relation quite implicit. > > The only reason to allocate all resync_pages together is for sake of efficiency, > and just for avoiding to allocate one resync_pages one time for each bio. > > We have to make .bi_private point to resync_pages(per bio), otherwise we > can't fetch pages into one bio at all, thinking about where to store the index > for each bio's pre-allocated pages, and it has to be per bio. So the reason is we can't find the corresponding pages of the bio if bi_private points to r1bio, right? Got it. We don't have many choices in this way. Ok, I don't insist. Please add some comments in the get_resync_r1bio to describe how the data structure is organized. Thanks, Shaohua
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index c442b4657e2f..900144f39630 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -77,6 +77,16 @@ static void lower_barrier(struct r1conf *conf, sector_t sector_nr); #define raid1_log(md, fmt, args...) \ do { if ((md)->queue) blk_add_trace_msg((md)->queue, "raid1 " fmt, ##args); } while (0) +static inline struct resync_pages *get_resync_pages(struct bio *bio) +{ + return bio->bi_private; +} + +static inline struct r1bio *get_resync_r1bio(struct bio *bio) +{ + return get_resync_pages(bio)->raid_bio; +} + static void * r1bio_pool_alloc(gfp_t gfp_flags, void *data) { struct pool_info *pi = data; @@ -104,12 +114,18 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) struct r1bio *r1_bio; struct bio *bio; int need_pages; - int i, j; + int j; + struct resync_pages *rps; r1_bio = r1bio_pool_alloc(gfp_flags, pi); if (!r1_bio) return NULL; + rps = kmalloc(sizeof(struct resync_pages) * pi->raid_disks, + gfp_flags); + if (!rps) + goto out_free_r1bio; + /* * Allocate bios : 1 for reading, n-1 for writing */ @@ -129,22 +145,22 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) need_pages = pi->raid_disks; else need_pages = 1; - for (j = 0; j < need_pages; j++) { + for (j = 0; j < pi->raid_disks; j++) { + struct resync_pages *rp = &rps[j]; + bio = r1_bio->bios[j]; - bio->bi_vcnt = RESYNC_PAGES; - - if (bio_alloc_pages(bio, gfp_flags)) - goto out_free_pages; - } - /* 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++) { - 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; - } + + if (j < need_pages) { + if (resync_alloc_pages(rp, gfp_flags)) + goto out_free_pages; + } else { + memcpy(rp, &rps[0], sizeof(*rp)); + resync_get_all_pages(rp); + } + + rp->idx = 0; + rp->raid_bio = r1_bio; + bio->bi_private = rp; } r1_bio->master_bio = NULL; @@ -153,11 +169,14 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) out_free_pages: while (--j >= 0) - bio_free_pages(r1_bio->bios[j]); + resync_free_pages(&rps[j]); out_free_bio: while (++j < pi->raid_disks) bio_put(r1_bio->bios[j]); + kfree(rps); + +out_free_r1bio: r1bio_pool_free(r1_bio, data); return NULL; } @@ -165,14 +184,18 @@ static void * r1buf_pool_alloc(gfp_t gfp_flags, void *data) static void r1buf_pool_free(void *__r1_bio, void *data) { struct pool_info *pi = data; - int i,j; + int i; struct r1bio *r1bio = __r1_bio; + struct resync_pages *rp = NULL; - 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++) + for (i = pi->raid_disks; i--; ) { + rp = get_resync_pages(r1bio->bios[i]); + resync_free_pages(rp); bio_put(r1bio->bios[i]); + } + + /* resync pages array stored in the 1st bio's .bi_private */ + kfree(rp); r1bio_pool_free(r1bio, data); } @@ -1849,7 +1872,7 @@ static int raid1_remove_disk(struct mddev *mddev, struct md_rdev *rdev) static void end_sync_read(struct bio *bio) { - struct r1bio *r1_bio = bio->bi_private; + struct r1bio *r1_bio = get_resync_r1bio(bio); update_head_pos(r1_bio->read_disk, r1_bio); @@ -1868,7 +1891,7 @@ static void end_sync_read(struct bio *bio) static void end_sync_write(struct bio *bio) { int uptodate = !bio->bi_error; - struct r1bio *r1_bio = bio->bi_private; + struct r1bio *r1_bio = get_resync_r1bio(bio); struct mddev *mddev = r1_bio->mddev; struct r1conf *conf = mddev->private; sector_t first_bad; @@ -2085,6 +2108,7 @@ static void process_checks(struct r1bio *r1_bio) int size; int error; struct bio *b = r1_bio->bios[i]; + struct resync_pages *rp = get_resync_pages(b); if (b->bi_end_io != end_sync_read) continue; /* fixup the bio for reuse, but preserve errno */ @@ -2097,7 +2121,8 @@ static void process_checks(struct r1bio *r1_bio) conf->mirrors[i].rdev->data_offset; b->bi_bdev = conf->mirrors[i].rdev->bdev; b->bi_end_io = end_sync_read; - b->bi_private = r1_bio; + rp->raid_bio = r1_bio; + b->bi_private = rp; size = b->bi_iter.bi_size; for (j = 0; j < vcnt ; j++) { @@ -2755,7 +2780,6 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr, for (i = 0; i < conf->raid_disks * 2; i++) { struct md_rdev *rdev; bio = r1_bio->bios[i]; - bio_reset(bio); rdev = rcu_dereference(conf->mirrors[i].rdev); if (rdev == NULL || @@ -2811,7 +2835,6 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr, atomic_inc(&rdev->nr_pending); bio->bi_iter.bi_sector = sector_nr + rdev->data_offset; bio->bi_bdev = rdev->bdev; - bio->bi_private = r1_bio; if (test_bit(FailFast, &rdev->flags)) bio->bi_opf |= MD_FAILFAST; } @@ -2899,7 +2922,7 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr, for (i = 0 ; i < conf->raid_disks * 2; i++) { bio = r1_bio->bios[i]; if (bio->bi_end_io) { - page = bio->bi_io_vec[bio->bi_vcnt].bv_page; + page = resync_fetch_page(get_resync_pages(bio)); /* * won't fail because the vec table is big @@ -2911,8 +2934,8 @@ static sector_t raid1_sync_request(struct mddev *mddev, sector_t sector_nr, nr_sectors += len>>9; sector_nr += len>>9; sync_blocks -= (len>>9); - } while (r1_bio->bios[disk]->bi_vcnt < RESYNC_PAGES); - bio_full: + } while (resync_page_available(r1_bio->bios[disk]->bi_private)); + r1_bio->sectors = nr_sectors; if (mddev_is_clustered(mddev) &&
Now we allocate one page array for managing resync pages, instead of using bio's vec table to do that, and the old way is very hacky and won't work any more if multipage bvec is enabled. The introduced cost is that we need to allocate (128 + 16) * raid_disks bytes per r1_bio, and it is fine because the inflight r1_bio for resync shouldn't be much, as pointed by Shaohua. Also the bio_reset() in raid1_sync_request() is removed because all bios are freshly new now and not necessary to reset any more. This patch can be thought as a cleanup too Suggested-by: Shaohua Li <shli@kernel.org> Signed-off-by: Ming Lei <tom.leiming@gmail.com> --- drivers/md/raid1.c | 83 ++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 53 insertions(+), 30 deletions(-)