Ceph » CephFS

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?

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?

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:

https://github.com/ceph/ceph/commit/f5e6b6ad19cbd60dea125818ee3490f7ba07567b#diff-6e3ae1ae909fa6dcdd03acd00100313b8933d5848f090a7213a6b77a8f2a56baR208

but it re-uses the link:

https://github.com/ceph/ceph/commit/f5e6b6ad19cbd60dea125818ee3490f7ba07567b#diff-258e472d088b256e5122b3d762383469a8afad46f09cb54a1de6fc0ab8a9508fR86

which is normally used for this list:

https://github.com/ceph/ceph/blob/f5e6b6ad19cbd60dea125818ee3490f7ba07567b/src/mds/SnapRealm.h#L55

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:

https://github.com/ceph/ceph/commit/f5e6b6ad19cbd60dea125818ee3490f7ba07567b#diff-6e3ae1ae909fa6dcdd03acd00100313b8933d5848f090a7213a6b77a8f2a56baR208

but it re-uses the link:

https://github.com/ceph/ceph/commit/f5e6b6ad19cbd60dea125818ee3490f7ba07567b#diff-258e472d088b256e5122b3d762383469a8afad46f09cb54a1de6fc0ab8a9508fR86

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.

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?

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?

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.