Bug #64659
openmds: switch to using xlists instead of elists
Added by Dhairya Parmar 2 months ago. Updated 22 days ago.
0%
Description
elist(size_t o) : _head(NULL), item_offset(o) {}
~elist() {
ceph_assert(_head.empty());
}
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0]. Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
while the `xlist` is comparatively simple:
xlist() : _front(0), _back(0), _size(0) {}
~xlist() {
ceph_assert(_size == 0);
ceph_assert(_front == 0);
ceph_assert(_back == 0);
}
Only MDS code makes use of elists as of now, I think it will be a good step to refactor MDS code to make use of xlist which will help prevent bugs like [0], make code simpler and efficient.
[0] https://tracker.ceph.com/issues/64008
[1] https://tracker.ceph.com/issues/64008#note-9
Updated by Patrick Donnelly 2 months ago
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
This is not a bug or a desirable change.
Updated by Dhairya Parmar 2 months ago
Patrick Donnelly wrote:
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
So structures being part of multiple elists is not a problem but using the same link/offset in multiple elists is?
This is not a bug or a desirable change.
Updated by Patrick Donnelly 2 months ago
Dhairya Parmar wrote:
Patrick Donnelly wrote:
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
So structures being part of multiple elists is not a problem but using the same link/offset in multiple elists is?
Yes. (It can be anyway. You would need guarantees that the ::item can only be used in one of the lists at any given time.)
Updated by Patrick Donnelly 2 months ago
- Related to Bug #64008: mds: CInode::item_caps used in two different lists added
Updated by Dhairya Parmar 2 months ago
Patrick Donnelly wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
So structures being part of multiple elists is not a problem but using the same link/offset in multiple elists is?
Yes. (It can be anyway. You would need guarantees that the ::item can only be used in one of the lists at any given time.)
So when these elists are replaced with xlists, there is no need keep track of "is this ::item used anywhere", since there isn't any dependency I find relevant to offsets in xlist.h. Take a look at this [0], this makes it so easy to use these items. Why not do the same with the MDS code
Updated by Venky Shankar 2 months ago
Dhairya Parmar wrote:
Patrick Donnelly wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
So structures being part of multiple elists is not a problem but using the same link/offset in multiple elists is?
Yes. (It can be anyway. You would need guarantees that the ::item can only be used in one of the lists at any given time.)
So when these elists are replaced with xlists, there is no need keep track of "is this ::item used anywhere", since there isn't any dependency I find relevant to offsets in xlist.h.
Are you sure? Could you explain this with an example?
Updated by Dhairya Parmar about 2 months ago
Venky Shankar wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
So structures being part of multiple elists is not a problem but using the same link/offset in multiple elists is?
Yes. (It can be anyway. You would need guarantees that the ::item can only be used in one of the lists at any given time.)
So when these elists are replaced with xlists, there is no need keep track of "is this ::item used anywhere", since there isn't any dependency I find relevant to offsets in xlist.h.
Are you sure? Could you explain this with an example?
Take a look at `item_revoking_caps` and `revoking_caps` in locker code. `revoking_caps` makes use of the item `item_revoking_caps` but since it is xlist which by definition doesn't need offset to be provided at the time of initialisation therefore there is no need of making sure there isn't any other linkage of `item_revoking_caps`.
circling to issue of tracker 64008 - if `inodes_with_caps` was an xlist, we dont need this initialisation [0]
[0] https://github.com/ceph/ceph/blob/fce2ded2fdb0b2b9aebc75ee28bf2fc0aa3d99ab/src/mds/SnapRealm.cc#L63
Updated by Venky Shankar about 2 months ago
Dhairya Parmar wrote:
Venky Shankar wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
So structures being part of multiple elists is not a problem but using the same link/offset in multiple elists is?
Yes. (It can be anyway. You would need guarantees that the ::item can only be used in one of the lists at any given time.)
So when these elists are replaced with xlists, there is no need keep track of "is this ::item used anywhere", since there isn't any dependency I find relevant to offsets in xlist.h.
Are you sure? Could you explain this with an example?
Take a look at `item_revoking_caps` and `revoking_caps` in locker code. `revoking_caps` makes use of the item `item_revoking_caps` but since it is xlist which by definition doesn't need offset to be provided at the time of initialisation therefore there is no need of making sure there isn't any other linkage of `item_revoking_caps`.
So, you are telling using xlist would avoid bugs like the one mentioned in #64008? What stops me from doing:
xlist<A*> list1; xlist<A*>::item item1; xlist<A*>::item item2; list1.push_back(item1); list1.push_back(item2);
?
Updated by Dhairya Parmar about 1 month ago
Venky Shankar wrote:
Dhairya Parmar wrote:
Venky Shankar wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
Dhairya Parmar wrote:
Patrick Donnelly wrote:
working with elist might lead to severe consequences at times if the same class member is used to initialise multiple elists e.g. [0].
That problem also exists for xlist.
Not only one needs to be highly careful while working with elist but it also induces space overhead where one might need to construct an extra elist to feed its offset the created elist so that no two elists make use of same elist.
It's the offset of the elist<T>::item that determines which list it belongs to. The offset for the ::item is fed to the elist() when it is constructed.
We do have structures like CInode which are part of multiple elist. Please do more research.
So structures being part of multiple elists is not a problem but using the same link/offset in multiple elists is?
Yes. (It can be anyway. You would need guarantees that the ::item can only be used in one of the lists at any given time.)
So when these elists are replaced with xlists, there is no need keep track of "is this ::item used anywhere", since there isn't any dependency I find relevant to offsets in xlist.h.
Are you sure? Could you explain this with an example?
Take a look at `item_revoking_caps` and `revoking_caps` in locker code. `revoking_caps` makes use of the item `item_revoking_caps` but since it is xlist which by definition doesn't need offset to be provided at the time of initialisation therefore there is no need of making sure there isn't any other linkage of `item_revoking_caps`.
So, you are telling using xlist would avoid bugs like the one mentioned in #64008? What stops me from doing:
[...]
?
Yes, I think it should. Exactly, if we make this across all the MDS code, we would not need to make initializations like `need_snapflush_inodes(member_offset(CInode, item_to_flush)), mds(m), mdcache(c) {}`