| Message ID | CAC-hyiFJSDtmgbSXn=N-mNKcuWuqiE9fPxtVz1fRFw4Q-j7HMg@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 02, 2012 at 03:48:14PM -0700, Yehuda Sadeh wrote: > On Mon, Jun 25, 2012 at 12:37 AM, Guangliang Zhao <gzhao@suse.com> wrote: > > Signed-off-by: Guangliang Zhao <gzhao@suse.com> > > --- > > drivers/block/rbd.c | 10 ++++------ > > 1 files changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > > index 65665c9..3d6dfc8 100644 > > --- a/drivers/block/rbd.c > > +++ b/drivers/block/rbd.c > > @@ -719,8 +719,6 @@ static struct bio *bio_chain_clone(struct bio **old, struct bio **next, > > goto err_out; > > > > if (total + old_chain->bi_size > len) { > > - struct bio_pair *bp; > > - > > /* > > * this split can only happen with a single paged bio, > > * split_bio will BUG_ON if this is not the case > > @@ -732,13 +730,13 @@ static struct bio *bio_chain_clone(struct bio **old, struct bio **next, > > > > /* split the bio. We'll release it either in the next > > call, or it will have to be released outside */ > > - bp = bio_split(old_chain, (len - total) / SECTOR_SIZE); > > - if (!bp) > > + *bp = bio_split(old_chain, (len - total) / SECTOR_SIZE); > > + if (!*bp) > > goto err_out; > > > > - __bio_clone(tmp, &bp->bio1); > > + __bio_clone(tmp, &(*bp)->bio1); > > > > - *next = &bp->bio2; > > + *next = &(*bp)->bio2; > > } else { > > __bio_clone(tmp, old_chain); > > *next = old_chain->bi_next; > > The above patch looks right, that is, fixes a leak. However, I'm not > too sure that we're not missing some bigger problem that was hidden > (due to the leak). Effectively we never called bio_pair_release() on > the bio_pair that we created in bio_chain_clone(). However, I think we > should only call that after we're done with sending the data. The bio_pair is only used to split the orgin bio and clone them to the tmps, no one will touch it again, so I think it is safe release it here. The bio_pair could be freed actually only after 3 times release, because the reference count of bio_pair is initialized to 3 when bio_split and bio_pair_release only drops the reference count. There are not enough release times for the bio_pair even apply either of these patches. I think transferring data with bio1 and bio2 of bio_pair instead of the tmp bio chain is a better way. It can not only save mem(for tmp bio chain), but also manage the reference count of bio_pair more comfortably(with callback of the bio1 and bio2). We can slove the above problems more easily with this approach. Thanks for you reply:)
On Mon, Jul 02, 2012 at 03:48:14PM -0700, Yehuda Sadeh wrote: > On Mon, Jun 25, 2012 at 12:37 AM, Guangliang Zhao <gzhao@suse.com> wrote: > > Signed-off-by: Guangliang Zhao <gzhao@suse.com> > > --- > > drivers/block/rbd.c | 10 ++++------ > > 1 files changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > > index 65665c9..3d6dfc8 100644 > > --- a/drivers/block/rbd.c > > +++ b/drivers/block/rbd.c > > @@ -719,8 +719,6 @@ static struct bio *bio_chain_clone(struct bio **old, struct bio **next, > > goto err_out; > > > > if (total + old_chain->bi_size > len) { > > - struct bio_pair *bp; > > - > > /* > > * this split can only happen with a single paged bio, > > * split_bio will BUG_ON if this is not the case > > @@ -732,13 +730,13 @@ static struct bio *bio_chain_clone(struct bio **old, struct bio **next, > > > > /* split the bio. We'll release it either in the next > > call, or it will have to be released outside */ > > - bp = bio_split(old_chain, (len - total) / SECTOR_SIZE); > > - if (!bp) > > + *bp = bio_split(old_chain, (len - total) / SECTOR_SIZE); > > + if (!*bp) > > goto err_out; > > > > - __bio_clone(tmp, &bp->bio1); > > + __bio_clone(tmp, &(*bp)->bio1); > > > > - *next = &bp->bio2; > > + *next = &(*bp)->bio2; > > } else { > > __bio_clone(tmp, old_chain); > > *next = old_chain->bi_next; > > The above patch looks right, that is, fixes a leak. However, I'm not > too sure that we're not missing some bigger problem that was hidden > (due to the leak). Effectively we never called bio_pair_release() on > the bio_pair that we created in bio_chain_clone(). However, I think we > should only call that after we're done with sending the data. The bio_pair is only used to split the orgin bio and clone them to the tmps, no one will touch it again, so I think it is safe release it here. The bio_pair could be freed actually only after 3 times release, because the reference count of bio_pair is initialized to 3 when bio_split and bio_pair_release only drops the reference count. There are not enough release times for the bio_pair even apply either of these patches. I think transferring data with bio1 and bio2 of bio_pair instead of the tmp bio chain is a better way. It can not only save mem(for tmp bio chain), but also manage the reference count of bio_pair more comfortably(with callback of the bio1 and bio2). We can slove the above problems more easily with this approach. Thanks for you reply:)
diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c index 013c7a5..7bf3e36 100644 --- a/drivers/block/rbd.c +++ b/drivers/block/rbd.c @@ -131,6 +131,7 @@ struct rbd_req_coll { */ struct rbd_request { struct request *rq; /* blk layer request */ + struct bio_pair *bp; struct bio *bio; /* cloned bio */ struct page **pages; /* list of used pages */ u64 len; @@ -867,6 +868,7 @@ static int rbd_do_request(struct request *rq, struct ceph_snap_context *snapc, u64 snapid, const char *obj, u64 ofs, u64 len, + struct bio_pair *bp, struct bio *bio, struct page **pages, int num_pages, @@ -918,6 +920,7 @@ static int rbd_do_request(struct request *rq, req->r_callback = rbd_cb; req_data->rq = rq; + req_data->bp = bp; req_data->bio = bio; req_data->pages = pages; req_data->len = len; @@ -968,6 +971,7 @@ static int rbd_do_request(struct request *rq, done_err: bio_chain_put(req_data->bio); + bio_pair_release(req_data->bp); ceph_osdc_put_request(req); done_pages: rbd_coll_end_req(req_data, ret, len); @@ -1010,6 +1014,9 @@ static void rbd_req_cb(struct ceph_osd_request *req, struct ceph_msg *msg) if (req_data->bio) bio_chain_put(req_data->bio); + if (req_data->bp) + bio_pair_release(req_data->bp); + ceph_osdc_put_request(req); kfree(req_data);