Hi Shaohua, On Wed, Mar 1, 2017 at 7:30 AM, Shaohua Li <s...@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.leim...@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