Sorry for the late response. Currently I'm quite busy.
In your earlier mail you described a possible dead lock.
With this patch applied, are you sure a similar deadlock cannot
happen?
Additionally, let's assume tcmu_vma_fault/tcmu_try_get_data_page
- after having found a valid page to map - is interrupted after
releasing the invalidate_lock. Are there any locks held to prevent
find_free_blocks from jumping in and possibly remove that page from
xarray and try to remove it from the mmapped area?
If not, we might end up mapping a no longer valid page.
Of course, this would be a long standing problem not caused by your
change. But if there would be a problem, we should try to fix it
when touching this code, I think.
Unfortunately I didn't manage yet to check which locks are involved
during page fault handling and unmap_mapping_range.
Bodo
On 16.03.22 11:43, Xiaoguang Wang wrote:
hello,
Gentle ping.
Regards,
Xiaoguang Wang
Currently tcmu_vma_fault() uses udev->cmdr_lock to avoid concurrent
find_free_blocks(), which unmaps idle pages and truncates them. This
work is really like many filesystem's truncate operations, but they
use address_space->invalidate_lock to protect race.
This patch replaces cmdr_lock with address_space->invalidate_lock in
tcmu fault procedure, which will also make page-fault have concurrency.
Signed-off-by: Xiaoguang Wang <xiaoguang.wang@xxxxxxxxxxxxxxxxx>
---
drivers/target/target_core_user.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/target/target_core_user.c
b/drivers/target/target_core_user.c
index 06a5c4086551..e0a62623ccd7 100644
--- a/drivers/target/target_core_user.c
+++ b/drivers/target/target_core_user.c
@@ -1815,13 +1815,14 @@ static int tcmu_find_mem_index(struct
vm_area_struct *vma)
static struct page *tcmu_try_get_data_page(struct tcmu_dev *udev,
uint32_t dpi)
{
+ struct address_space *mapping = udev->inode->i_mapping;
struct page *page;
- mutex_lock(&udev->cmdr_lock);
+ filemap_invalidate_lock_shared(mapping);
page = xa_load(&udev->data_pages, dpi);
if (likely(page)) {
get_page(page);
- mutex_unlock(&udev->cmdr_lock);
+ filemap_invalidate_unlock_shared(mapping);
return page;
}
@@ -1831,7 +1832,7 @@ static struct page
*tcmu_try_get_data_page(struct tcmu_dev *udev, uint32_t dpi)
*/
pr_err("Invalid addr to data page mapping (dpi %u) on device %s\n",
dpi, udev->name);
- mutex_unlock(&udev->cmdr_lock);
+ filemap_invalidate_unlock_shared(mapping);
return NULL;
}
@@ -3111,6 +3112,7 @@ static void find_free_blocks(void)
loff_t off;
u32 pages_freed, total_pages_freed = 0;
u32 start, end, block, total_blocks_freed = 0;
+ struct address_space *mapping;
if (atomic_read(&global_page_count) <= tcmu_global_max_pages)
return;
@@ -3134,6 +3136,7 @@ static void find_free_blocks(void)
continue;
}
+ mapping = udev->inode->i_mapping;
end = udev->dbi_max + 1;
block = find_last_bit(udev->data_bitmap, end);
if (block == udev->dbi_max) {
@@ -3152,12 +3155,14 @@ static void find_free_blocks(void)
udev->dbi_max = block;
}
+ filemap_invalidate_lock(mapping);
/* Here will truncate the data area from off */
off = udev->data_off + (loff_t)start * udev->data_blk_size;
- unmap_mapping_range(udev->inode->i_mapping, off, 0, 1);
+ unmap_mapping_range(mapping, off, 0, 1);
/* Release the block pages */
pages_freed = tcmu_blocks_release(udev, start, end - 1);
+ filemap_invalidate_unlock(mapping);
mutex_unlock(&udev->cmdr_lock);
total_pages_freed += pages_freed;
