cephfs: potential adjust failure in lru_expire #19277
Conversation
|
The first adjust() can move item from 'top' to 'bottom'. The second adjust() seems uesless. |
|
in the first adjust, the toplen and topwant never changed, |
|
so the item from toplist would not move to bottom list |
|
I think you are right. but we should call adjust() at very beginning of lru_get_next_expire(). because lru_pin() and lru_unpin() do not call adjust()/ |
|
I see your point, will add it to the beginning.. |
| // look through tail of bot | ||
| while (bottom.size()) { | ||
| LRUObject *p = bottom.back(); | ||
| if (!p->lru_pinned) return p; | ||
|
|
||
| // move to pintail | ||
| pintail.push_front(&p->lru_link); | ||
| adjust(); |
There was a problem hiding this comment.
I believe this adjust() is necessary because the size of bottom is changing so it may be necessary to move objects from top to bottom.
| @@ -174,7 +174,6 @@ class LRU { | |||
|
|
|||
| // move to pintail | |||
| pintail.push_front(&p->lru_link); | |||
| adjust(); | |||
There was a problem hiding this comment.
This loop probably isn't well tested because bottom will be empty if we are in this loop. I think you're right it may be possible for adjust() to crash if we remove an item from top here. Is that the problem you found originally? If that's the case, then we should fix adjust() to handle bottom.size() == 0, not remove this call here.
There was a problem hiding this comment.
int adjust():
uint64_t toplen = top.size();
uint64_t topwant = (midpoint * (double)(lru_get_size() - num_pinned));
both lru_get_size() and num_pinned do not change when moving item to pintail
There was a problem hiding this comment.
hi batrick
please refer to zheng's comment for the first adjust .
about the second adjust
I know if we could hit the second adjust we need to handle bottom_lru.size==0 in adjust. but the thing is i think the second adjust will never be hitted.
the only condition would make that happen is the bottom lru does not contain any unpined item and the top lru contains at least one pined item. according to the adjust algoritm, we can do some math, that condition would never happen. let's suppose we got a coner case by setting midpoint to 1.0, which means size of top lru would be the number of unpined item, and, since the bottom does not contain any unpined item , so the unpined items would be all in top lru, which proves there is no room for pinned item in top lru. so even this case we can not get that condition.
There was a problem hiding this comment.
hi batrick
please refer to zheng's comment for the first adjust .
about the second adjust
I know if we could hit the second adjust we need to handle bottom_lru.size==0 in adjust. but the thing is i think the second adjust will never be hitted.
the only condition would make that happen is the bottom lru does not contain any unpined item and the top lru contains at least one pined item. according to the adjust algoritm, we can do some math, that condition would never happen. let's suppose we got a coner case by setting midpoint to 1.0, which means size of top lru would be the number of unpined item, and, since the bottom does not contain any unpined item , so the unpined items would be all in top lru, which proves there is no room for pinned item in top lru. so even this case we can not get that condition.
|
You are right the I agree with you otherwise. Thanks for the explanation. |
|
@batrick do you mean i should remove the "adjust();" at the beginning of this function too ? |
|
No, nevermind. We don't call |
|
@batrick thank you ! |
Fix: the first adjust is no needed,it will never take real effect.
the second 'adjust' may never get the chance to be executed
suppose we can reach the second 'adjust', it will crash because the bottom list is empty now.
Fixes: http://tracker.ceph.com/issues/22458
Signed-off-by: dongdong tao <tdd21151186@gmail.com>
|
@batrick updated |
* refs/pull/19277/head: cephfs: potential adjust failure in lru_expire Reviewed-by: Patrick Donnelly <pdonnell@redhat.com> Reviewed-by: Zheng Yan <zyan@redhat.com>
Fix: the first 'adjust' is not needed,it will never take real effect.
the second 'adjust' may never get the chance to be executed
suppose we can reach the second 'adjust', it will crash because the bottom list is empty here.
Signed-off-by: dongdong tao tdd21151186@gmail.com