| Message ID | 1488296503-4987-5-git-send-email-tom.leiming@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Feb 28, 2017 at 11:41:34PM +0800, Ming Lei wrote: > Now resync I/O use bio's bec table to manage pages, > this way is very hacky, and may not work any more > once multipage bvec is introduced. > > So introduce helpers and new data structure for > managing resync I/O pages more cleanly. > > Signed-off-by: Ming Lei <tom.leiming@gmail.com> > --- > drivers/md/md.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 54 insertions(+) > > diff --git a/drivers/md/md.h b/drivers/md/md.h > index 1d63239a1be4..b5a638d85cb4 100644 > --- a/drivers/md/md.h > +++ b/drivers/md/md.h > @@ -720,4 +720,58 @@ static inline void mddev_check_writesame(struct mddev *mddev, struct bio *bio) > #define RESYNC_BLOCK_SIZE (64*1024) > #define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE) > > +/* for managing resync I/O pages */ > +struct resync_pages { > + unsigned idx; /* for get/put page from the pool */ > + void *raid_bio; > + struct page *pages[RESYNC_PAGES]; > +}; I'd like this to be embedded into r1bio directly. Not sure if we really need a structure. > + > +static inline int resync_alloc_pages(struct resync_pages *rp, > + gfp_t gfp_flags) > +{ > + int i; > + > + for (i = 0; i < RESYNC_PAGES; i++) { > + rp->pages[i] = alloc_page(gfp_flags); > + if (!rp->pages[i]) > + goto out_free; > + } > + > + return 0; > + > + out_free: > + while (--i >= 0) > + __free_page(rp->pages[i]); > + return -ENOMEM; > +} > + > +static inline void resync_free_pages(struct resync_pages *rp) > +{ > + int i; > + > + for (i = 0; i < RESYNC_PAGES; i++) > + __free_page(rp->pages[i]); Since we will use get_page, shouldn't this be put_page? > +} > + > +static inline void resync_get_all_pages(struct resync_pages *rp) > +{ > + int i; > + > + for (i = 0; i < RESYNC_PAGES; i++) > + get_page(rp->pages[i]); > +} > + > +static inline struct page *resync_fetch_page(struct resync_pages *rp) > +{ > + if (WARN_ON_ONCE(rp->idx >= RESYNC_PAGES)) > + return NULL; > + return rp->pages[rp->idx++]; I'd like the caller explicitly specify the index instead of a global variable to track it, which will make the code more understandable and less error prone. > +} > + > +static inline bool resync_page_available(struct resync_pages *rp) > +{ > + return rp->idx < RESYNC_PAGES; > +} Then we don't need this obscure API. Thanks, Shaohua
Hi Shaohua, On Wed, Mar 1, 2017 at 7:30 AM, Shaohua Li <shli@kernel.org> wrote: > On Tue, Feb 28, 2017 at 11:41:34PM +0800, Ming Lei wrote: >> Now resync I/O use bio's bec table to manage pages, >> this way is very hacky, and may not work any more >> once multipage bvec is introduced. >> >> So introduce helpers and new data structure for >> managing resync I/O pages more cleanly. >> >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> >> --- >> drivers/md/md.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 54 insertions(+) >> >> diff --git a/drivers/md/md.h b/drivers/md/md.h >> index 1d63239a1be4..b5a638d85cb4 100644 >> --- a/drivers/md/md.h >> +++ b/drivers/md/md.h >> @@ -720,4 +720,58 @@ static inline void mddev_check_writesame(struct mddev *mddev, struct bio *bio) >> #define RESYNC_BLOCK_SIZE (64*1024) >> #define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE) >> >> +/* for managing resync I/O pages */ >> +struct resync_pages { >> + unsigned idx; /* for get/put page from the pool */ >> + void *raid_bio; >> + struct page *pages[RESYNC_PAGES]; >> +}; > > I'd like this to be embedded into r1bio directly. Not sure if we really need a > structure. There are two reasons we can't put this into r1bio: - r1bio is used in both normal and resync I/O, not fair to allocate more in normal I/O, and that is why this patch wouldn't like to touch r1bio or r10bio - the count of 'struct resync_pages' instance depends on raid_disks(raid1) or copies(raid10), both can't be decided during compiling. > >> + >> +static inline int resync_alloc_pages(struct resync_pages *rp, >> + gfp_t gfp_flags) >> +{ >> + int i; >> + >> + for (i = 0; i < RESYNC_PAGES; i++) { >> + rp->pages[i] = alloc_page(gfp_flags); >> + if (!rp->pages[i]) >> + goto out_free; >> + } >> + >> + return 0; >> + >> + out_free: >> + while (--i >= 0) >> + __free_page(rp->pages[i]); >> + return -ENOMEM; >> +} >> + >> +static inline void resync_free_pages(struct resync_pages *rp) >> +{ >> + int i; >> + >> + for (i = 0; i < RESYNC_PAGES; i++) >> + __free_page(rp->pages[i]); > > Since we will use get_page, shouldn't this be put_page? You are right, will fix in v3. > >> +} >> + >> +static inline void resync_get_all_pages(struct resync_pages *rp) >> +{ >> + int i; >> + >> + for (i = 0; i < RESYNC_PAGES; i++) >> + get_page(rp->pages[i]); >> +} >> + >> +static inline struct page *resync_fetch_page(struct resync_pages *rp) >> +{ >> + if (WARN_ON_ONCE(rp->idx >= RESYNC_PAGES)) >> + return NULL; >> + return rp->pages[rp->idx++]; > > I'd like the caller explicitly specify the index instead of a global variable > to track it, which will make the code more understandable and less error prone. That is fine, but the index has to be per bio, and finally the index has to be stored in 'struct resync_pages', so every user has to call it in the following way: resync_fetch_page(rp, rp->idx); then looks no benefit to pass index explicitly. > >> +} >> + >> +static inline bool resync_page_available(struct resync_pages *rp) >> +{ >> + return rp->idx < RESYNC_PAGES; >> +} > > Then we don't need this obscure API. That is fine. Thanks, Ming Lei
On Thu, Mar 02, 2017 at 10:09:14AM +0800, Ming Lei wrote: > Hi Shaohua, > > On Wed, Mar 1, 2017 at 7:30 AM, Shaohua Li <shli@kernel.org> wrote: > > On Tue, Feb 28, 2017 at 11:41:34PM +0800, Ming Lei wrote: > >> Now resync I/O use bio's bec table to manage pages, > >> this way is very hacky, and may not work any more > >> once multipage bvec is introduced. > >> > >> So introduce helpers and new data structure for > >> managing resync I/O pages more cleanly. > >> > >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> > >> --- > >> drivers/md/md.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 54 insertions(+) > >> > >> diff --git a/drivers/md/md.h b/drivers/md/md.h > >> index 1d63239a1be4..b5a638d85cb4 100644 > >> --- a/drivers/md/md.h > >> +++ b/drivers/md/md.h > >> @@ -720,4 +720,58 @@ static inline void mddev_check_writesame(struct mddev *mddev, struct bio *bio) > >> #define RESYNC_BLOCK_SIZE (64*1024) > >> #define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE) > >> > >> +/* for managing resync I/O pages */ > >> +struct resync_pages { > >> + unsigned idx; /* for get/put page from the pool */ > >> + void *raid_bio; > >> + struct page *pages[RESYNC_PAGES]; > >> +}; > > > > I'd like this to be embedded into r1bio directly. Not sure if we really need a > > structure. > > There are two reasons we can't put this into r1bio: > - r1bio is used in both normal and resync I/O, not fair to allocate more > in normal I/O, and that is why this patch wouldn't like to touch r1bio or r10bio > > - the count of 'struct resync_pages' instance depends on raid_disks(raid1) > or copies(raid10), both can't be decided during compiling. Yes, I said it should be a pointer of r1bio pointing to the pages in other emails. > > > >> +} > >> + > >> +static inline bool resync_page_available(struct resync_pages *rp) > >> +{ > >> + return rp->idx < RESYNC_PAGES; > >> +} > > > > Then we don't need this obscure API. > > That is fine. > > > Thanks, > Ming Lei > > > >> + > >> +static inline int resync_alloc_pages(struct resync_pages *rp, > >> + gfp_t gfp_flags) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < RESYNC_PAGES; i++) { > >> + rp->pages[i] = alloc_page(gfp_flags); > >> + if (!rp->pages[i]) > >> + goto out_free; > >> + } > >> + > >> + return 0; > >> + > >> + out_free: > >> + while (--i >= 0) > >> + __free_page(rp->pages[i]); > >> + return -ENOMEM; > >> +} > >> + > >> +static inline void resync_free_pages(struct resync_pages *rp) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < RESYNC_PAGES; i++) > >> + __free_page(rp->pages[i]); > > > > Since we will use get_page, shouldn't this be put_page? > > You are right, will fix in v3. > > > > >> +} > >> + > >> +static inline void resync_get_all_pages(struct resync_pages *rp) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < RESYNC_PAGES; i++) > >> + get_page(rp->pages[i]); > >> +} > >> + > >> +static inline struct page *resync_fetch_page(struct resync_pages *rp) > >> +{ > >> + if (WARN_ON_ONCE(rp->idx >= RESYNC_PAGES)) > >> + return NULL; > >> + return rp->pages[rp->idx++]; > > > > I'd like the caller explicitly specify the index instead of a global variable > > to track it, which will make the code more understandable and less error prone. > > That is fine, but the index has to be per bio, and finally the index > has to be stored > in 'struct resync_pages', so every user has to call it in the following way: > > resync_fetch_page(rp, rp->idx); > > then looks no benefit to pass index explicitly. I suppose the callers always iterate though page 0 to RESYNC_PAGES - 1. Then the callers can use an index by themselves. That will clearly show which page the callers are using. The resync_fetch_page() wrap is a blackbox we always need to refer to the definition. Thanks, Shaohua
On Fri, Mar 3, 2017 at 1:55 AM, Shaohua Li <shli@kernel.org> wrote: > On Thu, Mar 02, 2017 at 10:09:14AM +0800, Ming Lei wrote: >> Hi Shaohua, >> >> On Wed, Mar 1, 2017 at 7:30 AM, Shaohua Li <shli@kernel.org> wrote: >> > On Tue, Feb 28, 2017 at 11:41:34PM +0800, Ming Lei wrote: >> >> Now resync I/O use bio's bec table to manage pages, >> >> this way is very hacky, and may not work any more >> >> once multipage bvec is introduced. >> >> >> >> So introduce helpers and new data structure for >> >> managing resync I/O pages more cleanly. >> >> >> >> Signed-off-by: Ming Lei <tom.leiming@gmail.com> >> >> --- >> >> drivers/md/md.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ >> >> 1 file changed, 54 insertions(+) >> >> >> >> diff --git a/drivers/md/md.h b/drivers/md/md.h >> >> index 1d63239a1be4..b5a638d85cb4 100644 >> >> --- a/drivers/md/md.h >> >> +++ b/drivers/md/md.h >> >> @@ -720,4 +720,58 @@ static inline void mddev_check_writesame(struct mddev *mddev, struct bio *bio) >> >> #define RESYNC_BLOCK_SIZE (64*1024) >> >> #define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE) >> >> >> >> +/* for managing resync I/O pages */ >> >> +struct resync_pages { >> >> + unsigned idx; /* for get/put page from the pool */ >> >> + void *raid_bio; >> >> + struct page *pages[RESYNC_PAGES]; >> >> +}; >> > >> > I'd like this to be embedded into r1bio directly. Not sure if we really need a >> > structure. >> >> There are two reasons we can't put this into r1bio: >> - r1bio is used in both normal and resync I/O, not fair to allocate more >> in normal I/O, and that is why this patch wouldn't like to touch r1bio or r10bio >> >> - the count of 'struct resync_pages' instance depends on raid_disks(raid1) >> or copies(raid10), both can't be decided during compiling. > > Yes, I said it should be a pointer of r1bio pointing to the pages in other emails. OK, got it, :-) Actually I did that in my local tree, and finally I figured out not necessary to introduce this pointor, which only usage is for freeing, and we can get the pointor from .bi_private of the 1st bio. And this stuff can be commented clearly. > >> > >> >> +} >> >> + >> >> +static inline bool resync_page_available(struct resync_pages *rp) >> >> +{ >> >> + return rp->idx < RESYNC_PAGES; >> >> +} >> > >> > Then we don't need this obscure API. >> >> That is fine. >> >> >> Thanks, >> Ming Lei > >> > >> >> + >> >> +static inline int resync_alloc_pages(struct resync_pages *rp, >> >> + gfp_t gfp_flags) >> >> +{ >> >> + int i; >> >> + >> >> + for (i = 0; i < RESYNC_PAGES; i++) { >> >> + rp->pages[i] = alloc_page(gfp_flags); >> >> + if (!rp->pages[i]) >> >> + goto out_free; >> >> + } >> >> + >> >> + return 0; >> >> + >> >> + out_free: >> >> + while (--i >= 0) >> >> + __free_page(rp->pages[i]); >> >> + return -ENOMEM; >> >> +} >> >> + >> >> +static inline void resync_free_pages(struct resync_pages *rp) >> >> +{ >> >> + int i; >> >> + >> >> + for (i = 0; i < RESYNC_PAGES; i++) >> >> + __free_page(rp->pages[i]); >> > >> > Since we will use get_page, shouldn't this be put_page? >> >> You are right, will fix in v3. >> >> > >> >> +} >> >> + >> >> +static inline void resync_get_all_pages(struct resync_pages *rp) >> >> +{ >> >> + int i; >> >> + >> >> + for (i = 0; i < RESYNC_PAGES; i++) >> >> + get_page(rp->pages[i]); >> >> +} >> >> + >> >> +static inline struct page *resync_fetch_page(struct resync_pages *rp) >> >> +{ >> >> + if (WARN_ON_ONCE(rp->idx >= RESYNC_PAGES)) >> >> + return NULL; >> >> + return rp->pages[rp->idx++]; >> > >> > I'd like the caller explicitly specify the index instead of a global variable >> > to track it, which will make the code more understandable and less error prone. >> >> That is fine, but the index has to be per bio, and finally the index >> has to be stored >> in 'struct resync_pages', so every user has to call it in the following way: >> >> resync_fetch_page(rp, rp->idx); >> >> then looks no benefit to pass index explicitly. > > I suppose the callers always iterate though page 0 to RESYNC_PAGES - 1. Then > the callers can use an index by themselves. That will clearly show which page > the callers are using. The resync_fetch_page() wrap is a blackbox we always > need to refer to the definition. OK, understood. Thanks, Ming Lei
diff --git a/drivers/md/md.h b/drivers/md/md.h index 1d63239a1be4..b5a638d85cb4 100644 --- a/drivers/md/md.h +++ b/drivers/md/md.h @@ -720,4 +720,58 @@ static inline void mddev_check_writesame(struct mddev *mddev, struct bio *bio) #define RESYNC_BLOCK_SIZE (64*1024) #define RESYNC_PAGES ((RESYNC_BLOCK_SIZE + PAGE_SIZE-1) / PAGE_SIZE) +/* for managing resync I/O pages */ +struct resync_pages { + unsigned idx; /* for get/put page from the pool */ + void *raid_bio; + struct page *pages[RESYNC_PAGES]; +}; + +static inline int resync_alloc_pages(struct resync_pages *rp, + gfp_t gfp_flags) +{ + int i; + + for (i = 0; i < RESYNC_PAGES; i++) { + rp->pages[i] = alloc_page(gfp_flags); + if (!rp->pages[i]) + goto out_free; + } + + return 0; + + out_free: + while (--i >= 0) + __free_page(rp->pages[i]); + return -ENOMEM; +} + +static inline void resync_free_pages(struct resync_pages *rp) +{ + int i; + + for (i = 0; i < RESYNC_PAGES; i++) + __free_page(rp->pages[i]); +} + +static inline void resync_get_all_pages(struct resync_pages *rp) +{ + int i; + + for (i = 0; i < RESYNC_PAGES; i++) + get_page(rp->pages[i]); +} + +static inline struct page *resync_fetch_page(struct resync_pages *rp) +{ + if (WARN_ON_ONCE(rp->idx >= RESYNC_PAGES)) + return NULL; + return rp->pages[rp->idx++]; +} + +static inline bool resync_page_available(struct resync_pages *rp) +{ + return rp->idx < RESYNC_PAGES; +} + #endif /* _MD_MD_H */
Now resync I/O use bio's bec table to manage pages, this way is very hacky, and may not work any more once multipage bvec is introduced. So introduce helpers and new data structure for managing resync I/O pages more cleanly. Signed-off-by: Ming Lei <tom.leiming@gmail.com> --- drivers/md/md.h | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+)