Project

General

Profile

Actions

Bug #2933

closed

rbd: bio_pair leak in bio_chain_clone()

Added by Alex Elder over 11 years ago. Updated over 11 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
Target version:
-
% Done:

0%

Source:
Development
Tags:
Backport:
Regression:
Severity:
Reviewed:
Affected Versions:
ceph-qa-suite:
Pull request ID:
Crash signature (v1):
Crash signature (v2):

Description

Guangliang Zhao <> pointed out this problem on the
mailing list. Here's the latest edition of his proposed fix:
http://www.spinics.net/lists/ceph-devel/msg08008.html

When bio_split() is called in rbd.c:bio_chain_clone() it returns
a bio_pair pointer. But what we do with it is not right, and as
a result the bio_pair is leaked.

All the other places callers use bio_split(), they are immediately
followed by a sequence, something like:

bp = bio_split(...)
make_request(dev, &bp->bio1);
make_request(dev, &bp->bio2);
bio_pair_release(bp);

When bio_split() is called, it sets the reference count on the
returned bio_pair to 3 and sets up the callbacks for bp->bio1
and bp->bio2 will have been set up so they result in a call to
bio_pair_release(). Thus, all three releases for the references
the bio_pair is initialized with are accounted for, and whichever
path drops the reference last will free the bio_pair structure.

Instead, the rbd code does something more like:

bp = bio_split(...);
__bio_clone(new, &bp->bio1);
...
bio_pair_release(bp);

I'm not even sure what the outcome of this is with respect to
the bio pair...

In any case the structure of the rbd code isn't quite like the
other callers of bio_split() so it may be a little more involved
get things to be done correctly. We call bio_split() in
bio_chain_clone(), which is used to clone an entire chain of
bios, whereas the other users of bio_split() are handling a
single bio.

It may be that one of the two bio chains resulting from the
bio_chain_clone() call should end with the first bio in the
bio_pair structure, and the other should begin with the
second one, and that will result in the desired behavior.

But it'll take a little study to make sure it gets done right.

We may get an updated patch from Guangliang Zhao; his last one
had at least one bug in it.

Actions #1

Updated by Alex Elder over 11 years ago

  • Assignee set to Alex Elder

Assigning this to me so I can find it again.

Actions #2

Updated by Alex Elder over 11 years ago

I'm sorry I haven't been updating this. Zhao has provided
several patches that aim to solve this problem. The latest
one, here: https://patchwork.kernel.org/patch/1413431/
appeared OK, but subsequent testing failed--not due to a
memory leak but due to some other problem. Running xfstests
over rbd repeatedly fails test #13 with this patch applied.

Actions #3

Updated by Alex Elder over 11 years ago

  • Status changed from New to 7

I have posted a patch for review that adjusts rbd_rq_fn() so it
uses a new function, bio_chain_clone_range(). The new function
has an easier-to-understand interface that uses no bio_pair
structures, and passes a minimal amount of extra information
between consecutive calls.

I have tested the result, and unlike a previous attempt I made
(and Zhao made) it passes all of the tests we run in the xfstests
suite over rbd. I also did tests with an rbd image set with an
artificially small object order (11--meaning a 2048 byte object
size, smaller than a page). This ensured that the code that
actually splits requests that span an object boundary got
exercised, and I was able to verify it was working as intended.

Actions #4

Updated by Alex Elder over 11 years ago

  • Status changed from 7 to Resolved

A fix for this has been committed to the ceph-client/testing
branch and has survived some days of testing.

commit f7760dad286829682a8d36f4563ab20a65732414
Author: Alex Elder <>
Date: Sat Oct 20 22:17:27 2012 -0500

rbd: simplify rbd_rq_fn()
Actions

Also available in: Atom PDF