Skip to content

mgr/volumes: fetch trash and clone entries without blocking volume access#33413

Merged
batrick merged 3 commits intoceph:masterfrom
vshankar:wip-44207
Feb 25, 2020
Merged

mgr/volumes: fetch trash and clone entries without blocking volume access#33413
batrick merged 3 commits intoceph:masterfrom
vshankar:wip-44207

Conversation

@vshankar
Copy link
Contributor

No description provided.

@vshankar vshankar added the cephfs Ceph File System label Feb 19, 2020
@vshankar vshankar requested review from ajarr and batrick February 19, 2020 14:30
Copy link
Contributor

@ajarr ajarr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't completely follow the dead lock issue. It's between the main command dispatcher thread and the purge queue thread? Looking at the remove_subvolume() method https://github.com/ceph/ceph/blob/master/src/pybind/mgr/volumes/fs/volume.py#L153
after acquiring volume lock, it renames the subvolume and kick starts the purge thread. It doesn't wait on the purge threads.

# verify trash dir is clean
self._wait_for_trash_empty()

def test_async_subvolume_rm_large(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test passed for me locally even without the change below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its a best effort test to validate the fix since its a lock acquisition race.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

btw, I think we can just stick to removing large number of subvolumes in the original async rm test.

self.mount_a.mount()

# verify trash dir is clean
self._wait_for_trash_empty(timeout=300)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug in _wait_for_trash_empty() method. It doesn't pass the timeout argument down to the_wait_for_dir_empty() method. Please correct that in this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ack

@vshankar
Copy link
Contributor Author

I don't completely follow the dead lock issue. It's between the main command dispatcher thread and the purge queue thread? Looking at the remove_subvolume() method https://github.com/ceph/ceph/blob/master/src/pybind/mgr/volumes/fs/volume.py#L153

right -- the command dispatcher thread calls ->queue_job() which acquires AsyncJobs::lock and the purge threads acquires this lock and accesses the volume in exclusive mode.

Saw a deadlock when deleting lot of subvolumes -- purge threads were
stuck in accessing global lock for volume access. This can happen
when there is a concurrent remove (which renames and signals the
purge threads) and a purge thread is just about to scan the trash
directory for entries.

For the fix, purge threads fetches entries by accessing the volume
in lockless mode. This is safe from functionality point-of-view as
the rename and directory scan is correctly handled by the filesystem.
Worst case the purge thread would pick up the trash entry on next
scan, never leaving a stale trash entry.

Signed-off-by: Venky Shankar <vshankar@redhat.com>
Signed-off-by: Venky Shankar <vshankar@redhat.com>
@vshankar
Copy link
Contributor Author

vshankar commented Feb 24, 2020

@batrick
Copy link
Member

batrick commented Feb 24, 2020

@batrick
Copy link
Member

batrick commented Feb 25, 2020

Test failures are because the file system was deleted and the cleanup connection thread hangs. Not related to this PR so I will merge. Thanks Venky!

@batrick batrick merged commit 0908d48 into ceph:master Feb 25, 2020
@vshankar
Copy link
Contributor Author

Test failures are because the file system was deleted and the cleanup connection thread hangs. Not related to this PR so I will merge. Thanks Venky!

Normally, the cleanup thread would know that the volume no longer exist. If the volume got removed after the cleanup thread fetched the fs handle, the thread can hang.

@vshankar vshankar deleted the wip-44207 branch February 25, 2020 03:11
@batrick
Copy link
Member

batrick commented Feb 25, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants