@neesingh-rh L4781 says that setting the repaired flag to true would prevent it's entry in damagetable. Seems like someone did address this issue in past but inode still sneaked into the damagetable?

@neesingh-rh L4781 says that setting the repaired flag to true would prevent it's entry in damagetable. Seems like someone did address this issue in past but inode still sneaked into the damagetable?

@dparmar18 Setting the repaired flag to true makes the ScrubStack.cc to realise that backtrace is repaired and hence, logs shows Inode Repaired but its not clearing the entry (earlier) from the damage table. That's why we have to manually erase it by damage rm command which shouldn't be. And that's issue which this PR fixes.

@neesingh-rh L4781 says that setting the repaired flag to true would prevent it's entry in damagetable. Seems like someone did address this issue in past but inode still sneaked into the damagetable?

@dparmar18 Setting the repaired flag to true makes the ScrubStack.cc to realise that backtrace is repaired and hence, logs shows Inode Repaired but its not clearing the entry (earlier) from the damage table. That's why we have to manually erase it by damage rm command which shouldn't be. And that's issue which this PR fixes.

Ah, that comment is a bit misleading. Thanks for the explanation!

I'm fine with the current patch too but I was just thinking we can simplify this:

DamageTable

damage_entry_id_t DamageTable::remove_damaged_entry(inodeno_t ino) {
	erase(remotes.find(ino)->second->id);
}

CInode

mdcache->mds->damage_table.remove_damaged_entry(in->ino());

What do you think? @neesingh-rh

I'm fine with the current patch too but I was just thinking we can simplify this:

DamageTable

damage_entry_id_t DamageTable::remove_damaged_entry(inodeno_t ino) {
	erase(remotes.find(ino)->second->id);
}

CInode

mdcache->mds->damage_table.remove_damaged_entry(in->ino());

What do you think? @neesingh-rh

@dparmar18 Yeah, it looks simpler. And I had thought about this but just to avoid confusion in CInode.cc between two function for erase(kind of) , I went with this.(Doesn't have any strong opinion) If u say second is better, will update. What say?

I'm fine with the current patch too but I was just thinking we can simplify this:
DamageTable

damage_entry_id_t DamageTable::remove_damaged_entry(inodeno_t ino) {
	erase(remotes.find(ino)->second->id);
}

CInode

mdcache->mds->damage_table.remove_damaged_entry(in->ino());

What do you think? @neesingh-rh

@dparmar18 Yeah, it looks simpler. And I had thought about this but just to avoid confusion in CInode.cc between two function for erase(kind of) , I went with this.(Doesn't have any strong opinion) If u say second is better, will update. What say?

@neesingh-rh This would introduce one more erase() call in CInode.cc right? Doesn't that lead to more confusion? Or I didn't understand correctly?

@neesingh-rh No test for this ?

@neesingh-rh No test for this ?

Will update with the tests soon.

I did run the teuthology after cdoing cleanups and here's the link:https://pulpito.ceph.com/neesingh-2023-11-23_10:04:20-fs-wip-neesingh-testing-231123-distro-default-smithi/

Could you explain what the changes are that fixes that test case failure reported in #48895 (review) ?

I did run the teuthology after cdoing cleanups and here's the link:https://pulpito.ceph.com/neesingh-2023-11-23_10:04:20-fs-wip-neesingh-testing-231123-distro-default-smithi/

Could you explain what the changes are that fixes that test case failure reported in #48895 (review) ?

As we discussed earlier that after debuging the code many times it seemed that there was no problem in the code, we need to check only for the test case.
There were two failures that were happening:

1. assert was failing on not finding MDS_DAMAGE in the get_mon_health() dict which got solved by adding sleep of some time cause addition of MDS_DAMAGE in the dict takes some time.
2. It was skipping the scrubbing second time when scrub start repair was run which is only responsible for removing the damage from damage list. When I looked the code in ScrubStack.cc, It skips the scrubbing when there's no change since last scrub unless the header contains force flag and in our case we need to run scrub start repair second time , so I added force in the scrub start repair for test_health_status_after_backtrace_repair

I did run the teuthology after cdoing cleanups and here's the link:https://pulpito.ceph.com/neesingh-2023-11-23_10:04:20-fs-wip-neesingh-testing-231123-distro-default-smithi/

Could you explain what the changes are that fixes that test case failure reported in #48895 (review) ?

As we discussed earlier that after debuging the code many times it seemed that there was no problem in the code, we need to check only for the test case. There were two failures that were happening:

1. assert was failing on not finding MDS_DAMAGE in the get_mon_health() dict which got solved by adding sleep of some time cause addition of MDS_DAMAGE in the dict takes some time.

maybe you can use wait_until_true() with a timeout?

2. It was skipping the scrubbing second time when scrub start repair was run which is only responsible for removing the damage from damage list. When I looked the code in ScrubStack.cc, It skips the scrubbing when there's no change since last scrub unless the header contains force flag and in our case we need to run scrub start repair second time , so I added force in the scrub start repair for test_health_status_after_backtrace_repair

I did run the teuthology after cdoing cleanups and here's the link:https://pulpito.ceph.com/neesingh-2023-11-23_10:04:20-fs-wip-neesingh-testing-231123-distro-default-smithi/

Could you explain what the changes are that fixes that test case failure reported in #48895 (review) ?

As we discussed earlier that after debuging the code many times it seemed that there was no problem in the code, we need to check only for the test case. There were two failures that were happening:

1. assert was failing on not finding MDS_DAMAGE in the get_mon_health() dict which got solved by adding sleep of some time cause addition of MDS_DAMAGE in the dict takes some time.

maybe you can use wait_until_true() with a timeout?

We can but if there's no harm lets stick to this

2. It was skipping the scrubbing second time when scrub start repair was run which is only responsible for removing the damage from damage list. When I looked the code in ScrubStack.cc, It skips the scrubbing when there's no change since last scrub unless the header contains force flag and in our case we need to run scrub start repair second time , so I added force in the scrub start repair for test_health_status_after_backtrace_repair

I did run the teuthology after cdoing cleanups and here's the link:https://pulpito.ceph.com/neesingh-2023-11-23_10:04:20-fs-wip-neesingh-testing-231123-distro-default-smithi/

Could you explain what the changes are that fixes that test case failure reported in #48895 (review) ?

As we discussed earlier that after debuging the code many times it seemed that there was no problem in the code, we need to check only for the test case. There were two failures that were happening:

1. assert was failing on not finding MDS_DAMAGE in the get_mon_health() dict which got solved by adding sleep of some time cause addition of MDS_DAMAGE in the dict takes some time.

maybe you can use wait_until_true() with a timeout?

We can but if there's no harm lets stick to this

I'm with @dparmar18 on this one. Using wait_until_true() is the preferred way rather than using sleep.

Otherwise LGTM.

Wait, its failing after the latest changes.

jenkins test windows

Run link: https://pulpito.ceph.com/neesingh-2023-11-29_05:06:25-fs-wip-neesingh-testing-231123-distro-default-smithi/

jenkins test make check arm64

fs+smoke suite - https://pulpito.ceph.com/?branch=wip-vshankar-testing-20231206.125818