Bug #64008
openmds: CInode::item_caps used in two different lists
0%
Description
Since
https://github.com/ceph/ceph/commit/f5e6b6ad19cbd60dea125818ee3490f7ba07567b
I'm not sure why Zheng did it this way but it looks like a bug (or provides fuel for bugs).
Updated by Venky Shankar 3 months ago
- Assignee set to Dhairya Parmar
- Backport set to quincy,reef
Updated by Dhairya Parmar 3 months ago
can i get some more context on the problem here? also the mentioned commit has no trace of the list inodes_with_caps, am i missing something?
Updated by Dhairya Parmar 3 months ago
so we have `elist<CInode*> inodes_with_caps` in SnapRealm and `xlist<Inode*> inodes_with_caps` in ClientSnapRealm, I.e. same name for two different lists, is this the issue?
Updated by Patrick Donnelly 3 months ago
Dhairya Parmar wrote:
can i get some more context on the problem here? also the mentioned commit has no trace of the list inodes_with_caps, am i missing something?
The commit in the issue description adds a new elist:
but it re-uses the link:
which is normally used for this list:
https://github.com/ceph/ceph/blob/f5e6b6ad19cbd60dea125818ee3490f7ba07567b/src/mds/SnapRealm.h#L55
Updated by Dhairya Parmar 3 months ago
Patrick Donnelly wrote:
Dhairya Parmar wrote:
can i get some more context on the problem here? also the mentioned commit has no trace of the list inodes_with_caps, am i missing something?
The commit in the issue description adds a new elist:
but it re-uses the link:
which is normally used for this list:
https://github.com/ceph/ceph/blob/f5e6b6ad19cbd60dea125818ee3490f7ba07567b/src/mds/SnapRealm.h#L55
I get it now, so the Locker::need_snapflush_inodes and SnapRealm::inodes_with_caps both make use of CInode::item_caps at the time of initialisation.
Updated by Dhairya Parmar 3 months ago
- Subject changed from mds: CInode::inodes_with_caps used in two different lists to mds: CInode::item_caps used in two different lists
Updated by Dhairya Parmar 3 months ago
it seems the way it is implemented, `Locker::need_snapflush_inodes` is tightly coupled with CInode::item_caps, since I see insertion/deletion of item_caps in the commit mentioned. Should we maintain a list something like CInode::snapflush_caps that is basically a copy of CInode::item_caps?
Updated by Dhairya Parmar 3 months ago
also what kinds of bugs is this prone to? Locker and SnapRealm making changes at the same time? We have locks so this should be sorted, right?
Updated by Dhairya Parmar 2 months ago
It seems like only the MDS code to be using elist, why not just switch to using xlist? That way we completely avoid the need to feeding CInode::item_caps offset to two different elists and so this bug. This might require some extra efforts but in long term this should be beneficial. @venky @patrick @Greg Farnum any thoughts?
If this sounds to be a bit overkill then I'll move ahead with creating a new elist in CInode and initialise Locker::need_snapflush_inodes with it, but this way we also incur added space complexity that could've been avoided if we go ahead with the refactoring mentioned above.
Updated by Patrick Donnelly 2 months ago
- Related to Bug #64659: mds: switch to using xlists instead of elists added
Updated by Venky Shankar about 2 months ago
- Backport changed from quincy,reef to quincy,reef,squid
Updated by Patrick Donnelly about 2 months ago
- Status changed from New to Fix Under Review
- Target version changed from v19.0.0 to v20.0.0
- Backport changed from quincy,reef,squid to reef,squid
Updated by Venky Shankar about 1 month ago
- Status changed from Fix Under Review to Pending Backport
Let's hold off the backport till we get a cleaner run for fs suite in main branch.
Updated by Backport Bot about 1 month ago
- Copied to Backport #65315: reef: mds: CInode::item_caps used in two different lists added
Updated by Backport Bot about 1 month ago
- Copied to Backport #65316: squid: mds: CInode::item_caps used in two different lists added