[2659] | 1 | From 54c0ee608f4afd2b178c9b60eabfc3564293d996 Mon Sep 17 00:00:00 2001 |
---|
[2649] | 2 | From: Andrew Deason <adeason@sinenomine.net> |
---|
| 3 | Date: Sun, 14 Sep 2014 14:10:11 -0500 |
---|
| 4 | Subject: [PATCH] afs: Fix some afs_conn overcounts |
---|
| 5 | |
---|
| 6 | The usual pattern of using afs_Conn looks like this: |
---|
| 7 | |
---|
| 8 | do { |
---|
| 9 | tc = afs_Conn(...); |
---|
| 10 | if (tc) { |
---|
| 11 | code = /* ... */ |
---|
| 12 | } else { |
---|
| 13 | code = -1; |
---|
| 14 | } |
---|
| 15 | } while (afs_Analyze(...)); |
---|
| 16 | |
---|
| 17 | The afs_Analyze call, amongst other things, puts back the reference to |
---|
| 18 | the connection obtained from afs_Conn. If anything inside the do/while |
---|
| 19 | block exits that block without calling afs_Analyze or afs_PutConn, we |
---|
| 20 | will leak a reference to the conn. |
---|
| 21 | |
---|
| 22 | A few places currently do this, by jumping out of the loop with |
---|
| 23 | 'goto's. Specifically, in afs_dcache.c and afs_bypasscache.c. These |
---|
| 24 | locations currently leak references to our connection object (and to |
---|
| 25 | the underlying Rx connection object), which can cause problems over |
---|
| 26 | time. Specifically, this can cause a panic when the refcount overflows |
---|
| 27 | and becomes negative, causing a panic message that looks like: |
---|
| 28 | |
---|
| 29 | afs_PutConn: refcount imbalance 0xd34db33f -32768 |
---|
| 30 | |
---|
| 31 | To avoid this, make sure we afs_PutConn in these cases where we 'goto' |
---|
| 32 | out of the afs_Conn/afs_Analyze loop. Perhaps ideally we should cause |
---|
| 33 | afs_Analyze itself to be called in these situations, but for now just |
---|
| 34 | fix the problem with the least amount of impact possible. |
---|
| 35 | |
---|
| 36 | FIXES 131885 |
---|
| 37 | |
---|
| 38 | Change-Id: I3a52f8ccef24f01d04c02db0a4b711405360e323 |
---|
[2659] | 39 | Reviewed-on: http://gerrit.openafs.org/11464 |
---|
| 40 | Reviewed-by: Benjamin Kaduk <kaduk@mit.edu> |
---|
| 41 | Reviewed-by: Daria Brashear <shadow@your-file-system.com> |
---|
| 42 | Tested-by: Benjamin Kaduk <kaduk@mit.edu> |
---|
| 43 | Reviewed-by: Jeffrey Altman <jaltman@your-file-system.com> |
---|
[2649] | 44 | --- |
---|
| 45 | src/afs/afs_bypasscache.c | 1 + |
---|
| 46 | src/afs/afs_dcache.c | 7 +++++++ |
---|
| 47 | 2 files changed, 8 insertions(+) |
---|
| 48 | |
---|
| 49 | diff --git a/src/afs/afs_bypasscache.c b/src/afs/afs_bypasscache.c |
---|
[2659] | 50 | index f452638..4c6fb9a 100644 |
---|
[2649] | 51 | --- a/src/afs/afs_bypasscache.c |
---|
| 52 | +++ b/src/afs/afs_bypasscache.c |
---|
[2659] | 53 | @@ -621,6 +621,7 @@ afs_PrefetchNoCache(struct vcache *avc, |
---|
[2649] | 54 | } else { |
---|
| 55 | afs_warn("BYPASS: StartRXAFS_FetchData failed: %d\n", code); |
---|
| 56 | unlock_and_release_pages(auio); |
---|
| 57 | + afs_PutConn(tc, rxconn, SHARED_LOCK); |
---|
| 58 | goto done; |
---|
| 59 | } |
---|
| 60 | if (code == 0) { |
---|
| 61 | diff --git a/src/afs/afs_dcache.c b/src/afs/afs_dcache.c |
---|
[2659] | 62 | index 4a9edbd..338e8db 100644 |
---|
[2649] | 63 | --- a/src/afs/afs_dcache.c |
---|
| 64 | +++ b/src/afs/afs_dcache.c |
---|
[2659] | 65 | @@ -2398,6 +2398,13 @@ afs_GetDCache(struct vcache *avc, afs_size_t abyte, |
---|
[2649] | 66 | afs_PutDCache(tdc); |
---|
| 67 | tdc = 0; |
---|
| 68 | ReleaseReadLock(&avc->lock); |
---|
| 69 | + |
---|
| 70 | + if (tc) { |
---|
| 71 | + /* If we have a connection, we must put it back, |
---|
| 72 | + * since afs_Analyze will not be called here. */ |
---|
| 73 | + afs_PutConn(tc, rxconn, SHARED_LOCK); |
---|
| 74 | + } |
---|
| 75 | + |
---|
| 76 | slowPass = 1; |
---|
| 77 | goto RetryGetDCache; |
---|
| 78 | } |
---|
| 79 | -- |
---|
[2659] | 80 | 2.2.1 |
---|
[2649] | 81 | |
---|