Re: [PATCH v2 04/13] md: prepare for managing resync I/O pages in clean way
On Fri, Mar 3, 2017 at 1:55 AM, Shaohua Liwrote: > 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 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 >> >> --- >> >> 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 { >> >> + unsignedidx;/* 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
Re: [PATCH v2 04/13] md: prepare for managing resync I/O pages in clean way
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 Liwrote: > > 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 > >> --- > >> 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 { > >> + unsignedidx;/* 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
Re: [PATCH v2 04/13] md: prepare for managing resync I/O pages in clean way
Hi Shaohua, On Wed, Mar 1, 2017 at 7:30 AM, Shaohua Liwrote: > 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 >> --- >> 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 { >> + unsignedidx;/* 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
Re: [PATCH v2 04/13] md: prepare for managing resync I/O pages in clean way
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> --- > 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 { > + unsignedidx;/* 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