Re: [PATCH 2/2] scsi: target: tcmu: Use address_space->invalidate_lock

[Bodo Stroesser <bostroesser@gmail.com>]

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@linux.alibaba.com>
>> ---
>>   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;
>