| Message ID | 20170712082912.491-1-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 12, 2017 at 04:29:12PM +0800, Ming Lei wrote: > We will support multipage bvec soon, so initialize bvec > table using the standardy way instead of writing the > talbe directly. Otherwise it won't work any more once > multipage bvec is enabled. It seems to me like these callsites also should use bio_init instead of bio_reset to get a clean slate (and eventually phase out bio_reset), which should probably got into the helper as well. E.g. instead of pretending to preserve things we should simply build a new bio here.
On Wed, Jul 12 2017, Ming Lei wrote: > We will support multipage bvec soon, so initialize bvec > table using the standardy way instead of writing the > talbe directly. Otherwise it won't work any more once > multipage bvec is enabled. > > Acked-by: Guoqing Jiang <gqjiang@suse.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/md/md.c | 21 +++++++++++++++++++++ > drivers/md/md.h | 3 +++ > drivers/md/raid1.c | 16 ++-------------- > drivers/md/raid10.c | 4 ++-- > 4 files changed, 28 insertions(+), 16 deletions(-) > > diff --git a/drivers/md/md.c b/drivers/md/md.c > index 8cdca0296749..cc8dcd928dde 100644 > --- a/drivers/md/md.c > +++ b/drivers/md/md.c > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr) > } > EXPORT_SYMBOL(md_reload_sb); > > +/* generally called after bio_reset() for reseting bvec */ > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > + int size) > +{ > + int idx = 0; > + > + /* initialize bvec table again */ > + do { > + struct page *page = resync_fetch_page(rp, idx); > + int len = min_t(int, size, PAGE_SIZE); > + > + /* > + * won't fail because the vec table is big > + * enough to hold all these pages > + */ > + bio_add_page(bio, page, len, 0); > + size -= len; > + } while (idx++ < RESYNC_PAGES && size > 0); > +} > +EXPORT_SYMBOL(md_bio_reset_resync_pages); I really don't think this is a good idea. This code is specific to raid1/raid10. It is not generic md code. So it doesn't belong here. If you want to remove code duplication, then work on moving all raid1 functionality into raid10.c, then discard raid1.c Or at the very least, have a separate "raid1-10.c" file for the common code. NeilBrown > + > #ifndef MODULE > > /* > diff --git a/drivers/md/md.h b/drivers/md/md.h > index 2c780aa8d07f..efb32ce7a2f1 100644 > --- a/drivers/md/md.h > +++ b/drivers/md/md.h > @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp, > return NULL; > return rp->pages[idx]; > } > + > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > + int size); > #endif /* _MD_MD_H */ > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > index 7901ddc3362f..5dc3fda2fdf7 100644 > --- a/drivers/md/raid1.c > +++ b/drivers/md/raid1.c > @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio) > /* Fix variable parts of all bios */ > vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9); > for (i = 0; i < conf->raid_disks * 2; i++) { > - int j; > - int size; > blk_status_t status; > - struct bio_vec *bi; > struct bio *b = r1_bio->bios[i]; > struct resync_pages *rp = get_resync_pages(b); > if (b->bi_end_io != end_sync_read) > @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio) > status = b->bi_status; > bio_reset(b); > b->bi_status = status; > - b->bi_vcnt = vcnt; > - b->bi_iter.bi_size = r1_bio->sectors << 9; > b->bi_iter.bi_sector = r1_bio->sector + > conf->mirrors[i].rdev->data_offset; > b->bi_bdev = conf->mirrors[i].rdev->bdev; > @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio) > rp->raid_bio = r1_bio; > b->bi_private = rp; > > - size = b->bi_iter.bi_size; > - bio_for_each_segment_all(bi, b, j) { > - bi->bv_offset = 0; > - if (size > PAGE_SIZE) > - bi->bv_len = PAGE_SIZE; > - else > - bi->bv_len = size; > - size -= PAGE_SIZE; > - } > + /* initialize bvec table again */ > + md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9); > } > for (primary = 0; primary < conf->raid_disks * 2; primary++) > if (r1_bio->bios[primary]->bi_end_io == end_sync_read && > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > index e594ca610f27..cb8e803cd1c2 100644 > --- a/drivers/md/raid10.c > +++ b/drivers/md/raid10.c > @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) > rp = get_resync_pages(tbio); > bio_reset(tbio); > > - tbio->bi_vcnt = vcnt; > - tbio->bi_iter.bi_size = fbio->bi_iter.bi_size; > + md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size); > + > rp->raid_bio = r10_bio; > tbio->bi_private = rp; > tbio->bi_iter.bi_sector = r10_bio->devs[i].addr; > -- > 2.9.4
On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote: > On Wed, Jul 12 2017, Ming Lei wrote: > > > We will support multipage bvec soon, so initialize bvec > > table using the standardy way instead of writing the > > talbe directly. Otherwise it won't work any more once > > multipage bvec is enabled. > > > > Acked-by: Guoqing Jiang <gqjiang@suse.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/md/md.c | 21 +++++++++++++++++++++ > > drivers/md/md.h | 3 +++ > > drivers/md/raid1.c | 16 ++-------------- > > drivers/md/raid10.c | 4 ++-- > > 4 files changed, 28 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/md/md.c b/drivers/md/md.c > > index 8cdca0296749..cc8dcd928dde 100644 > > --- a/drivers/md/md.c > > +++ b/drivers/md/md.c > > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr) > > } > > EXPORT_SYMBOL(md_reload_sb); > > > > +/* generally called after bio_reset() for reseting bvec */ > > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > > + int size) > > +{ > > + int idx = 0; > > + > > + /* initialize bvec table again */ > > + do { > > + struct page *page = resync_fetch_page(rp, idx); > > + int len = min_t(int, size, PAGE_SIZE); > > + > > + /* > > + * won't fail because the vec table is big > > + * enough to hold all these pages > > + */ > > + bio_add_page(bio, page, len, 0); > > + size -= len; > > + } while (idx++ < RESYNC_PAGES && size > 0); > > +} > > +EXPORT_SYMBOL(md_bio_reset_resync_pages); > > I really don't think this is a good idea. > This code is specific to raid1/raid10. It is not generic md code. So > it doesn't belong here. We already added 'struct resync_pages' in drivers/md/md.h, so I think it is reasonable to add this function into drivers/md/md.c > > If you want to remove code duplication, then work on moving all raid1 > functionality into raid10.c, then discard raid1.c This patch is for avoiding new code duplication, not for removing current duplication. > > Or at the very least, have a separate "raid1-10.c" file for the common > code. You suggested it last time, but looks too overkill to be taken. But if someone wants to refactor raid1 and raid10, I think it can be a good start, but still not belong to this patch. Thanks, Ming > > NeilBrown > > > + > > #ifndef MODULE > > > > /* > > diff --git a/drivers/md/md.h b/drivers/md/md.h > > index 2c780aa8d07f..efb32ce7a2f1 100644 > > --- a/drivers/md/md.h > > +++ b/drivers/md/md.h > > @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp, > > return NULL; > > return rp->pages[idx]; > > } > > + > > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > > + int size); > > #endif /* _MD_MD_H */ > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c > > index 7901ddc3362f..5dc3fda2fdf7 100644 > > --- a/drivers/md/raid1.c > > +++ b/drivers/md/raid1.c > > @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio) > > /* Fix variable parts of all bios */ > > vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9); > > for (i = 0; i < conf->raid_disks * 2; i++) { > > - int j; > > - int size; > > blk_status_t status; > > - struct bio_vec *bi; > > struct bio *b = r1_bio->bios[i]; > > struct resync_pages *rp = get_resync_pages(b); > > if (b->bi_end_io != end_sync_read) > > @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio) > > status = b->bi_status; > > bio_reset(b); > > b->bi_status = status; > > - b->bi_vcnt = vcnt; > > - b->bi_iter.bi_size = r1_bio->sectors << 9; > > b->bi_iter.bi_sector = r1_bio->sector + > > conf->mirrors[i].rdev->data_offset; > > b->bi_bdev = conf->mirrors[i].rdev->bdev; > > @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio) > > rp->raid_bio = r1_bio; > > b->bi_private = rp; > > > > - size = b->bi_iter.bi_size; > > - bio_for_each_segment_all(bi, b, j) { > > - bi->bv_offset = 0; > > - if (size > PAGE_SIZE) > > - bi->bv_len = PAGE_SIZE; > > - else > > - bi->bv_len = size; > > - size -= PAGE_SIZE; > > - } > > + /* initialize bvec table again */ > > + md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9); > > } > > for (primary = 0; primary < conf->raid_disks * 2; primary++) > > if (r1_bio->bios[primary]->bi_end_io == end_sync_read && > > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c > > index e594ca610f27..cb8e803cd1c2 100644 > > --- a/drivers/md/raid10.c > > +++ b/drivers/md/raid10.c > > @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) > > rp = get_resync_pages(tbio); > > bio_reset(tbio); > > > > - tbio->bi_vcnt = vcnt; > > - tbio->bi_iter.bi_size = fbio->bi_iter.bi_size; > > + md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size); > > + > > rp->raid_bio = r10_bio; > > tbio->bi_private = rp; > > tbio->bi_iter.bi_sector = r10_bio->devs[i].addr; > > -- > > 2.9.4
On Wed, Jul 12, 2017 at 02:16:08AM -0700, Christoph Hellwig wrote: > On Wed, Jul 12, 2017 at 04:29:12PM +0800, Ming Lei wrote: > > We will support multipage bvec soon, so initialize bvec > > table using the standardy way instead of writing the > > talbe directly. Otherwise it won't work any more once > > multipage bvec is enabled. > > It seems to me like these callsites also should use bio_init > instead of bio_reset to get a clean slate (and eventually > phase out bio_reset), which should probably got into > the helper as well. E.g. instead of pretending to preserve > things we should simply build a new bio here. At least for resetting bvec table, bio_reset() is enough, so I think changing to bio_init() should belong to another topic, :-) Thanks, Ming
On Thu, Jul 13 2017, Ming Lei wrote: > On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote: >> On Wed, Jul 12 2017, Ming Lei wrote: >> >> > We will support multipage bvec soon, so initialize bvec >> > table using the standardy way instead of writing the >> > talbe directly. Otherwise it won't work any more once >> > multipage bvec is enabled. >> > >> > Acked-by: Guoqing Jiang <gqjiang@suse.com> >> > Signed-off-by: Ming Lei <ming.lei@redhat.com> >> > --- >> > drivers/md/md.c | 21 +++++++++++++++++++++ >> > drivers/md/md.h | 3 +++ >> > drivers/md/raid1.c | 16 ++-------------- >> > drivers/md/raid10.c | 4 ++-- >> > 4 files changed, 28 insertions(+), 16 deletions(-) >> > >> > diff --git a/drivers/md/md.c b/drivers/md/md.c >> > index 8cdca0296749..cc8dcd928dde 100644 >> > --- a/drivers/md/md.c >> > +++ b/drivers/md/md.c >> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr) >> > } >> > EXPORT_SYMBOL(md_reload_sb); >> > >> > +/* generally called after bio_reset() for reseting bvec */ >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, >> > + int size) >> > +{ >> > + int idx = 0; >> > + >> > + /* initialize bvec table again */ >> > + do { >> > + struct page *page = resync_fetch_page(rp, idx); >> > + int len = min_t(int, size, PAGE_SIZE); >> > + >> > + /* >> > + * won't fail because the vec table is big >> > + * enough to hold all these pages >> > + */ >> > + bio_add_page(bio, page, len, 0); >> > + size -= len; >> > + } while (idx++ < RESYNC_PAGES && size > 0); >> > +} >> > +EXPORT_SYMBOL(md_bio_reset_resync_pages); >> >> I really don't think this is a good idea. >> This code is specific to raid1/raid10. It is not generic md code. So >> it doesn't belong here. > > We already added 'struct resync_pages' in drivers/md/md.h, so I think > it is reasonable to add this function into drivers/md/md.c Alternative perspective: it was unreasonable to add "resync_pages" to md.h. > >> >> If you want to remove code duplication, then work on moving all raid1 >> functionality into raid10.c, then discard raid1.c > > This patch is for avoiding new code duplication, not for removing current > duplication. > >> >> Or at the very least, have a separate "raid1-10.c" file for the common >> code. > > You suggested it last time, but looks too overkill to be taken. But if > someone wants to refactor raid1 and raid10, I think it can be a good start, > but still not belong to this patch. You are trying to create common code for raid1 and raid10. This does not belong in md.c. If you really want to have a single copy of common code, then it exactly is the role of this patch to create a place to put it. I'm not saying you should put all common code in raid1-10.c. Just the function that you have identified. NeilBrown > > Thanks, > Ming > >> >> NeilBrown >> >> > + >> > #ifndef MODULE >> > >> > /* >> > diff --git a/drivers/md/md.h b/drivers/md/md.h >> > index 2c780aa8d07f..efb32ce7a2f1 100644 >> > --- a/drivers/md/md.h >> > +++ b/drivers/md/md.h >> > @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp, >> > return NULL; >> > return rp->pages[idx]; >> > } >> > + >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, >> > + int size); >> > #endif /* _MD_MD_H */ >> > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c >> > index 7901ddc3362f..5dc3fda2fdf7 100644 >> > --- a/drivers/md/raid1.c >> > +++ b/drivers/md/raid1.c >> > @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio) >> > /* Fix variable parts of all bios */ >> > vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9); >> > for (i = 0; i < conf->raid_disks * 2; i++) { >> > - int j; >> > - int size; >> > blk_status_t status; >> > - struct bio_vec *bi; >> > struct bio *b = r1_bio->bios[i]; >> > struct resync_pages *rp = get_resync_pages(b); >> > if (b->bi_end_io != end_sync_read) >> > @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio) >> > status = b->bi_status; >> > bio_reset(b); >> > b->bi_status = status; >> > - b->bi_vcnt = vcnt; >> > - b->bi_iter.bi_size = r1_bio->sectors << 9; >> > b->bi_iter.bi_sector = r1_bio->sector + >> > conf->mirrors[i].rdev->data_offset; >> > b->bi_bdev = conf->mirrors[i].rdev->bdev; >> > @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio) >> > rp->raid_bio = r1_bio; >> > b->bi_private = rp; >> > >> > - size = b->bi_iter.bi_size; >> > - bio_for_each_segment_all(bi, b, j) { >> > - bi->bv_offset = 0; >> > - if (size > PAGE_SIZE) >> > - bi->bv_len = PAGE_SIZE; >> > - else >> > - bi->bv_len = size; >> > - size -= PAGE_SIZE; >> > - } >> > + /* initialize bvec table again */ >> > + md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9); >> > } >> > for (primary = 0; primary < conf->raid_disks * 2; primary++) >> > if (r1_bio->bios[primary]->bi_end_io == end_sync_read && >> > diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c >> > index e594ca610f27..cb8e803cd1c2 100644 >> > --- a/drivers/md/raid10.c >> > +++ b/drivers/md/raid10.c >> > @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) >> > rp = get_resync_pages(tbio); >> > bio_reset(tbio); >> > >> > - tbio->bi_vcnt = vcnt; >> > - tbio->bi_iter.bi_size = fbio->bi_iter.bi_size; >> > + md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size); >> > + >> > rp->raid_bio = r10_bio; >> > tbio->bi_private = rp; >> > tbio->bi_iter.bi_sector = r10_bio->devs[i].addr; >> > -- >> > 2.9.4 > > > > -- > Ming
On Thu, Jul 13, 2017 at 01:09:28PM +1000, NeilBrown wrote: > On Thu, Jul 13 2017, Ming Lei wrote: > > > On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote: > >> On Wed, Jul 12 2017, Ming Lei wrote: > >> > >> > We will support multipage bvec soon, so initialize bvec > >> > table using the standardy way instead of writing the > >> > talbe directly. Otherwise it won't work any more once > >> > multipage bvec is enabled. > >> > > >> > Acked-by: Guoqing Jiang <gqjiang@suse.com> > >> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > >> > --- > >> > drivers/md/md.c | 21 +++++++++++++++++++++ > >> > drivers/md/md.h | 3 +++ > >> > drivers/md/raid1.c | 16 ++-------------- > >> > drivers/md/raid10.c | 4 ++-- > >> > 4 files changed, 28 insertions(+), 16 deletions(-) > >> > > >> > diff --git a/drivers/md/md.c b/drivers/md/md.c > >> > index 8cdca0296749..cc8dcd928dde 100644 > >> > --- a/drivers/md/md.c > >> > +++ b/drivers/md/md.c > >> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr) > >> > } > >> > EXPORT_SYMBOL(md_reload_sb); > >> > > >> > +/* generally called after bio_reset() for reseting bvec */ > >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > >> > + int size) > >> > +{ > >> > + int idx = 0; > >> > + > >> > + /* initialize bvec table again */ > >> > + do { > >> > + struct page *page = resync_fetch_page(rp, idx); > >> > + int len = min_t(int, size, PAGE_SIZE); > >> > + > >> > + /* > >> > + * won't fail because the vec table is big > >> > + * enough to hold all these pages > >> > + */ > >> > + bio_add_page(bio, page, len, 0); > >> > + size -= len; > >> > + } while (idx++ < RESYNC_PAGES && size > 0); > >> > +} > >> > +EXPORT_SYMBOL(md_bio_reset_resync_pages); > >> > >> I really don't think this is a good idea. > >> This code is specific to raid1/raid10. It is not generic md code. So > >> it doesn't belong here. > > > > We already added 'struct resync_pages' in drivers/md/md.h, so I think > > it is reasonable to add this function into drivers/md/md.c > > Alternative perspective: it was unreasonable to add "resync_pages" to > md.h. > > > > >> > >> If you want to remove code duplication, then work on moving all raid1 > >> functionality into raid10.c, then discard raid1.c > > > > This patch is for avoiding new code duplication, not for removing current > > duplication. > > > >> > >> Or at the very least, have a separate "raid1-10.c" file for the common > >> code. > > > > You suggested it last time, but looks too overkill to be taken. But if > > someone wants to refactor raid1 and raid10, I think it can be a good start, > > but still not belong to this patch. > > You are trying to create common code for raid1 and raid10. This does > not belong in md.c. > If you really want to have a single copy of common code, then it exactly > is the role of this patch to create a place to put it. > I'm not saying you should put all common code in raid1-10.c. Just the > function that you have identified. I really don't want to waste time on this kind of thing, I can do either one frankly. Shaohua, could you share me which way you like to merge? I can do it in either way.
On Thu, Jul 13, 2017 at 05:20:52PM +0800, Ming Lei wrote: > On Thu, Jul 13, 2017 at 01:09:28PM +1000, NeilBrown wrote: > > On Thu, Jul 13 2017, Ming Lei wrote: > > > > > On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote: > > >> On Wed, Jul 12 2017, Ming Lei wrote: > > >> > > >> > We will support multipage bvec soon, so initialize bvec > > >> > table using the standardy way instead of writing the > > >> > talbe directly. Otherwise it won't work any more once > > >> > multipage bvec is enabled. > > >> > > > >> > Acked-by: Guoqing Jiang <gqjiang@suse.com> > > >> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > >> > --- > > >> > drivers/md/md.c | 21 +++++++++++++++++++++ > > >> > drivers/md/md.h | 3 +++ > > >> > drivers/md/raid1.c | 16 ++-------------- > > >> > drivers/md/raid10.c | 4 ++-- > > >> > 4 files changed, 28 insertions(+), 16 deletions(-) > > >> > > > >> > diff --git a/drivers/md/md.c b/drivers/md/md.c > > >> > index 8cdca0296749..cc8dcd928dde 100644 > > >> > --- a/drivers/md/md.c > > >> > +++ b/drivers/md/md.c > > >> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr) > > >> > } > > >> > EXPORT_SYMBOL(md_reload_sb); > > >> > > > >> > +/* generally called after bio_reset() for reseting bvec */ > > >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, > > >> > + int size) > > >> > +{ > > >> > + int idx = 0; > > >> > + > > >> > + /* initialize bvec table again */ > > >> > + do { > > >> > + struct page *page = resync_fetch_page(rp, idx); > > >> > + int len = min_t(int, size, PAGE_SIZE); > > >> > + > > >> > + /* > > >> > + * won't fail because the vec table is big > > >> > + * enough to hold all these pages > > >> > + */ > > >> > + bio_add_page(bio, page, len, 0); > > >> > + size -= len; > > >> > + } while (idx++ < RESYNC_PAGES && size > 0); > > >> > +} > > >> > +EXPORT_SYMBOL(md_bio_reset_resync_pages); > > >> > > >> I really don't think this is a good idea. > > >> This code is specific to raid1/raid10. It is not generic md code. So > > >> it doesn't belong here. > > > > > > We already added 'struct resync_pages' in drivers/md/md.h, so I think > > > it is reasonable to add this function into drivers/md/md.c > > > > Alternative perspective: it was unreasonable to add "resync_pages" to > > md.h. > > > > > > > >> > > >> If you want to remove code duplication, then work on moving all raid1 > > >> functionality into raid10.c, then discard raid1.c > > > > > > This patch is for avoiding new code duplication, not for removing current > > > duplication. > > > > > >> > > >> Or at the very least, have a separate "raid1-10.c" file for the common > > >> code. > > > > > > You suggested it last time, but looks too overkill to be taken. But if > > > someone wants to refactor raid1 and raid10, I think it can be a good start, > > > but still not belong to this patch. > > > > You are trying to create common code for raid1 and raid10. This does > > not belong in md.c. > > If you really want to have a single copy of common code, then it exactly > > is the role of this patch to create a place to put it. > > I'm not saying you should put all common code in raid1-10.c. Just the > > function that you have identified. > > I really don't want to waste time on this kind of thing, I can do > either one frankly. > > Shaohua, could you share me which way you like to merge? I can do it in > either way. I don't have strong preference, but Neil's suggestion does make the code a little better. Of course, only put the function into the raid1-10.c right now. Thanks, Shaohua
On Thu, Jul 13 2017, Shaohua Li wrote: > On Thu, Jul 13, 2017 at 05:20:52PM +0800, Ming Lei wrote: >> On Thu, Jul 13, 2017 at 01:09:28PM +1000, NeilBrown wrote: >> > On Thu, Jul 13 2017, Ming Lei wrote: >> > >> > > On Thu, Jul 13, 2017 at 10:01:33AM +1000, NeilBrown wrote: >> > >> On Wed, Jul 12 2017, Ming Lei wrote: >> > >> >> > >> > We will support multipage bvec soon, so initialize bvec >> > >> > table using the standardy way instead of writing the >> > >> > talbe directly. Otherwise it won't work any more once >> > >> > multipage bvec is enabled. >> > >> > >> > >> > Acked-by: Guoqing Jiang <gqjiang@suse.com> >> > >> > Signed-off-by: Ming Lei <ming.lei@redhat.com> >> > >> > --- >> > >> > drivers/md/md.c | 21 +++++++++++++++++++++ >> > >> > drivers/md/md.h | 3 +++ >> > >> > drivers/md/raid1.c | 16 ++-------------- >> > >> > drivers/md/raid10.c | 4 ++-- >> > >> > 4 files changed, 28 insertions(+), 16 deletions(-) >> > >> > >> > >> > diff --git a/drivers/md/md.c b/drivers/md/md.c >> > >> > index 8cdca0296749..cc8dcd928dde 100644 >> > >> > --- a/drivers/md/md.c >> > >> > +++ b/drivers/md/md.c >> > >> > @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr) >> > >> > } >> > >> > EXPORT_SYMBOL(md_reload_sb); >> > >> > >> > >> > +/* generally called after bio_reset() for reseting bvec */ >> > >> > +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, >> > >> > + int size) >> > >> > +{ >> > >> > + int idx = 0; >> > >> > + >> > >> > + /* initialize bvec table again */ >> > >> > + do { >> > >> > + struct page *page = resync_fetch_page(rp, idx); >> > >> > + int len = min_t(int, size, PAGE_SIZE); >> > >> > + >> > >> > + /* >> > >> > + * won't fail because the vec table is big >> > >> > + * enough to hold all these pages >> > >> > + */ >> > >> > + bio_add_page(bio, page, len, 0); >> > >> > + size -= len; >> > >> > + } while (idx++ < RESYNC_PAGES && size > 0); >> > >> > +} >> > >> > +EXPORT_SYMBOL(md_bio_reset_resync_pages); >> > >> >> > >> I really don't think this is a good idea. >> > >> This code is specific to raid1/raid10. It is not generic md code. So >> > >> it doesn't belong here. >> > > >> > > We already added 'struct resync_pages' in drivers/md/md.h, so I think >> > > it is reasonable to add this function into drivers/md/md.c >> > >> > Alternative perspective: it was unreasonable to add "resync_pages" to >> > md.h. >> > >> > > >> > >> >> > >> If you want to remove code duplication, then work on moving all raid1 >> > >> functionality into raid10.c, then discard raid1.c >> > > >> > > This patch is for avoiding new code duplication, not for removing current >> > > duplication. >> > > >> > >> >> > >> Or at the very least, have a separate "raid1-10.c" file for the common >> > >> code. >> > > >> > > You suggested it last time, but looks too overkill to be taken. But if >> > > someone wants to refactor raid1 and raid10, I think it can be a good start, >> > > but still not belong to this patch. >> > >> > You are trying to create common code for raid1 and raid10. This does >> > not belong in md.c. >> > If you really want to have a single copy of common code, then it exactly >> > is the role of this patch to create a place to put it. >> > I'm not saying you should put all common code in raid1-10.c. Just the >> > function that you have identified. >> >> I really don't want to waste time on this kind of thing, I can do >> either one frankly. >> >> Shaohua, could you share me which way you like to merge? I can do it in >> either way. > > I don't have strong preference, but Neil's suggestion does make the code a > little better. Of course, only put the function into the raid1-10.c right now. To make it as easy as possible, I would suggest creating raid1-10.c containing just this function (and maybe the definitions from md.h), and declare the function "static" and #include raid1-10.c into raid1.c and raid10.c. i.e. no worrying about modules and exporting symbols. Anyone who cares (and that might even be me) could move functionality gradually out of raid1.c and raid10.c in raid1-10.c. Maybe where would come a tipping-point where it is easy to just discard raid1.c and raid10.c and finish the job. Thanks, NeilBrown
diff --git a/drivers/md/md.c b/drivers/md/md.c index 8cdca0296749..cc8dcd928dde 100644 --- a/drivers/md/md.c +++ b/drivers/md/md.c @@ -9130,6 +9130,27 @@ void md_reload_sb(struct mddev *mddev, int nr) } EXPORT_SYMBOL(md_reload_sb); +/* generally called after bio_reset() for reseting bvec */ +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, + int size) +{ + int idx = 0; + + /* initialize bvec table again */ + do { + struct page *page = resync_fetch_page(rp, idx); + int len = min_t(int, size, PAGE_SIZE); + + /* + * won't fail because the vec table is big + * enough to hold all these pages + */ + bio_add_page(bio, page, len, 0); + size -= len; + } while (idx++ < RESYNC_PAGES && size > 0); +} +EXPORT_SYMBOL(md_bio_reset_resync_pages); + #ifndef MODULE /* diff --git a/drivers/md/md.h b/drivers/md/md.h index 2c780aa8d07f..efb32ce7a2f1 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -782,4 +782,7 @@ static inline struct page *resync_fetch_page(struct resync_pages *rp, return NULL; return rp->pages[idx]; } + +void md_bio_reset_resync_pages(struct bio *bio, struct resync_pages *rp, + int size); #endif /* _MD_MD_H */ diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index 7901ddc3362f..5dc3fda2fdf7 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -2085,10 +2085,7 @@ static void process_checks(struct r1bio *r1_bio) /* Fix variable parts of all bios */ vcnt = (r1_bio->sectors + PAGE_SIZE / 512 - 1) >> (PAGE_SHIFT - 9); for (i = 0; i < conf->raid_disks * 2; i++) { - int j; - int size; blk_status_t status; - struct bio_vec *bi; struct bio *b = r1_bio->bios[i]; struct resync_pages *rp = get_resync_pages(b); if (b->bi_end_io != end_sync_read) @@ -2097,8 +2094,6 @@ static void process_checks(struct r1bio *r1_bio) status = b->bi_status; bio_reset(b); b->bi_status = status; - b->bi_vcnt = vcnt; - b->bi_iter.bi_size = r1_bio->sectors << 9; b->bi_iter.bi_sector = r1_bio->sector + conf->mirrors[i].rdev->data_offset; b->bi_bdev = conf->mirrors[i].rdev->bdev; @@ -2106,15 +2101,8 @@ static void process_checks(struct r1bio *r1_bio) rp->raid_bio = r1_bio; b->bi_private = rp; - size = b->bi_iter.bi_size; - bio_for_each_segment_all(bi, b, j) { - bi->bv_offset = 0; - if (size > PAGE_SIZE) - bi->bv_len = PAGE_SIZE; - else - bi->bv_len = size; - size -= PAGE_SIZE; - } + /* initialize bvec table again */ + md_bio_reset_resync_pages(b, rp, r1_bio->sectors << 9); } for (primary = 0; primary < conf->raid_disks * 2; primary++) if (r1_bio->bios[primary]->bi_end_io == end_sync_read && diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index e594ca610f27..cb8e803cd1c2 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -2086,8 +2086,8 @@ static void sync_request_write(struct mddev *mddev, struct r10bio *r10_bio) rp = get_resync_pages(tbio); bio_reset(tbio); - tbio->bi_vcnt = vcnt; - tbio->bi_iter.bi_size = fbio->bi_iter.bi_size; + md_bio_reset_resync_pages(tbio, rp, fbio->bi_iter.bi_size); + rp->raid_bio = r10_bio; tbio->bi_private = rp; tbio->bi_iter.bi_sector = r10_bio->devs[i].addr;