| Message ID | a4ff33f2c114d0d0e1d1f41b7524078d2e9fd3f0.1498762065.git.shli@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 29, 2017 at 11:54:17AM -0700, Shaohua Li wrote: > bio_free isn't a good place to free cgroup info. There are a > lot of cases bio is allocated in special way (for example, in stack) and > never gets called by bio_put hence bio_free, we are leaking memory. This > patch moves the free to bio endio, which should be called anyway. The > __bio_free call in bio_free is kept, in case the bio never gets called > bio endio. Jens already renamed __bio_free to bio_uninit in Linus' tree. That being said I think we should just kill __bio_free once all the work is in. But I think we should defer this post the initial pull for 4.12-rc to reduce the merge pain.
On 06/29/2017 02:24 PM, Christoph Hellwig wrote: > On Thu, Jun 29, 2017 at 11:54:17AM -0700, Shaohua Li wrote: >> bio_free isn't a good place to free cgroup info. There are a >> lot of cases bio is allocated in special way (for example, in stack) and >> never gets called by bio_put hence bio_free, we are leaking memory. This >> patch moves the free to bio endio, which should be called anyway. The >> __bio_free call in bio_free is kept, in case the bio never gets called >> bio endio. > > Jens already renamed __bio_free to bio_uninit in Linus' tree. > > That being said I think we should just kill __bio_free once all > the work is in. But I think we should defer this post the > initial pull for 4.12-rc to reduce the merge pain. Yes, let's pick up that work after the existing for-4.13/block has been merged upstream.
On Thu, Jun 29, 2017 at 03:23:47PM -0600, Jens Axboe wrote: > On 06/29/2017 02:24 PM, Christoph Hellwig wrote: > > On Thu, Jun 29, 2017 at 11:54:17AM -0700, Shaohua Li wrote: > >> bio_free isn't a good place to free cgroup info. There are a > >> lot of cases bio is allocated in special way (for example, in stack) and > >> never gets called by bio_put hence bio_free, we are leaking memory. This > >> patch moves the free to bio endio, which should be called anyway. The > >> __bio_free call in bio_free is kept, in case the bio never gets called > >> bio endio. > > > > Jens already renamed __bio_free to bio_uninit in Linus' tree. > > > > That being said I think we should just kill __bio_free once all > > the work is in. But I think we should defer this post the > > initial pull for 4.12-rc to reduce the merge pain. > > Yes, let's pick up that work after the existing for-4.13/block has > been merged upstream. Jens, should I repost the patch or you can take care of it (eg, change __bio_free to bio_uninit in the patch)? Thanks, Shaohua
On 07/10/2017 12:25 PM, Shaohua Li wrote: > On Thu, Jun 29, 2017 at 03:23:47PM -0600, Jens Axboe wrote: >> On 06/29/2017 02:24 PM, Christoph Hellwig wrote: >>> On Thu, Jun 29, 2017 at 11:54:17AM -0700, Shaohua Li wrote: >>>> bio_free isn't a good place to free cgroup info. There are a >>>> lot of cases bio is allocated in special way (for example, in stack) and >>>> never gets called by bio_put hence bio_free, we are leaking memory. This >>>> patch moves the free to bio endio, which should be called anyway. The >>>> __bio_free call in bio_free is kept, in case the bio never gets called >>>> bio endio. >>> >>> Jens already renamed __bio_free to bio_uninit in Linus' tree. >>> >>> That being said I think we should just kill __bio_free once all >>> the work is in. But I think we should defer this post the >>> initial pull for 4.12-rc to reduce the merge pain. >> >> Yes, let's pick up that work after the existing for-4.13/block has >> been merged upstream. > > Jens, > should I repost the patch or you can take care of it (eg, change __bio_free to > bio_uninit in the patch)? Can you repost it, please?
diff --git a/block/bio.c b/block/bio.c index 9cf98b2..e8cda9c 100644 --- a/block/bio.c +++ b/block/bio.c @@ -1828,6 +1828,8 @@ void bio_endio(struct bio *bio) } blk_throtl_bio_endio(bio); + /* release cgroup info */ + __bio_free(bio); if (bio->bi_end_io) bio->bi_end_io(bio); }
bio_free isn't a good place to free cgroup info. There are a lot of cases bio is allocated in special way (for example, in stack) and never gets called by bio_put hence bio_free, we are leaking memory. This patch moves the free to bio endio, which should be called anyway. The __bio_free call in bio_free is kept, in case the bio never gets called bio endio. This assumes ->bi_end_io() doesn't access cgroup info, which seems true in my audit. This along with Christoph's integrity patch should fix the memory leak issue. Signed-off-by: Shaohua Li <shli@fb.com> --- block/bio.c | 2 ++ 1 file changed, 2 insertions(+)