Message ID | 20170710041304.GB15321@ming.t460p (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Mon, Jul 10 2017, Ming Lei wrote: > On Mon, Jul 10, 2017 at 11:35:12AM +0800, Ming Lei wrote: >> On Mon, Jul 10, 2017 at 7:09 AM, NeilBrown <neilb@suse.com> wrote: ... >> >> + >> >> + rp->idx = 0; >> > >> > This is the only place the ->idx is initialized, in r1buf_pool_alloc(). >> > The mempool alloc function is suppose to allocate memory, not initialize >> > it. >> > >> > If the mempool_alloc() call cannot allocate memory it will use memory >> > from the pool. If this memory has already been used, then it will no >> > longer have the initialized value. >> > >> > In short: you need to initialise memory *after* calling >> > mempool_alloc(), unless you ensure it is reset to the init values before >> > calling mempool_free(). >> > >> > https://bugzilla.kernel.org/show_bug.cgi?id=196307 >> >> OK, thanks for posting it out. >> >> Another fix might be to reinitialize the variable(rp->idx = 0) in >> r1buf_pool_free(). >> Or just set it as zero every time when it is used. >> >> But I don't understand why mempool_free() calls pool->free() at the end of >> this function, which may cause to run pool->free() on a new allocated buf, >> seems a bug in mempool? > > Looks I missed the 'return' in mempool_free(), so it is fine. > > How about the following fix? It looks like it would probably work, but it is rather unusual to initialise something just before freeing it. Couldn't you just move the initialization to shortly after the mempool_alloc() call. There looks like a good place that already loops over all the bios.... Thanks, NeilBrown
diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c index e1a7e3d4c5e4..d31b06da3e3d 100644 --- a/drivers/md/raid1.c +++ b/drivers/md/raid1.c @@ -242,6 +242,7 @@ static void put_buf(struct r1bio *r1_bio) struct bio *bio = r1_bio->bios[i]; if (bio->bi_end_io) rdev_dec_pending(conf->mirrors[i].rdev, r1_bio->mddev); + get_resync_pages(bio)->idx = 0; } mempool_free(r1_bio, conf->r1buf_pool); diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c index 797ed60abd5e..c61523768745 100644 --- a/drivers/md/raid10.c +++ b/drivers/md/raid10.c @@ -299,12 +299,21 @@ static void free_r10bio(struct r10bio *r10_bio) mempool_free(r10_bio, conf->r10bio_pool); } -static void put_buf(struct r10bio *r10_bio) +static void free_r10bio_buf(struct r10bio *r10_bio, struct r10conf *conf) { - struct r10conf *conf = r10_bio->mddev->private; + int j; + + for (j = conf->copies; j--; ) + get_resync_pages(r10_bio->devs[j].bio)->idx = 0; mempool_free(r10_bio, conf->r10buf_pool); +} + +static void put_buf(struct r10bio *r10_bio) +{ + struct r10conf *conf = r10_bio->mddev->private; + free_r10bio_buf(r10_bio, conf); lower_barrier(conf); } @@ -4383,7 +4392,7 @@ static sector_t reshape_request(struct mddev *mddev, sector_t sector_nr, * on all the target devices. */ // FIXME - mempool_free(r10_bio, conf->r10buf_pool); + free_r10bio_buf(r10_bio, conf); set_bit(MD_RECOVERY_INTR, &mddev->recovery); return sectors_done; }