Re: [RFC PATCH v2 02/51] KVM: guest_memfd: Introduce and use shareability to guard faulting

[Ackerley Tng]

Ira Weiny <ira.weiny@xxxxxxxxx> writes:

> 
> [...snip...]
> 
>> >> 
>> >> Conversions take the filemap_invalidate_lock() too, along with
>> >> allocations, truncations.
>> >> 
>> >> Because the filemap_invalidate_lock() might be reused for other
>> >> fs-specific operations, I didn't do the mt_set_external_lock() thing to
>> >> lock at a low level to avoid nested locking or special maple tree code
>> >> to avoid taking the lock on other paths.
>> >
>> > I don't think using the filemap_invalidate_lock() is going to work well
>> > here.  I've had some hangs on it in my testing and experiments.  I think
>> > it is better to specifically lock the state tracking itself.  I believe
>> > Michael mentioned this as well in a previous thread.
>> 
>> Definitely took the big hammer lock for a start and might be optimizable.
>> 
>> Considerations so far: when a conversion is happening, these have to be
>> locked out:
>> 
>> + Conversions from competing threads
>
> Agreed.  And this needs filemap_invalidate_lock() as well as the maple
> tree lock.
>
> Call this item 1.
>
>> + Allocations in kvm_gmem_fault_user_mapping(), because whether an
>>   offset can be faulted depends on the outcome of conversion
>
> Agreed.  And this needs filemap_invalidate_lock() as well as the maple
> tree lock.
>
> Call this item 2.
>
>> + Allocations (fallocate() or kvm_gmem_get_pfn()) and truncations,
>>   because conversions (for now) involves removing a folio from the
>>   filemap, restructuring, then restoring to the filemap, and
>>     + Allocations should reuse a folio that was already in the filemap
>>     + Truncations remove a folio, and should not skip removal of a folio
>>       because it was taken out just for conversion
>
> I don't think this is required...
>
>> + memory failure handling, where we don't remove folios from the
>>   filemap, but we might restructure, to split huge folios to just unmap
>>   pages with failed memory
>
> ... nor this.  These don't change the sharability maple tree.
>
> These operations don't change or need to know the shareability AFAICT.
>
> Merging a folio would have to check the maple tree to ensure we don't
> merge incompatible folios...  But that is a read check and should be easy
> to add.
>
>> I think essentially because conversion involves restructuring, and
>> restructuring involves filemap operations and other filemap operations
>> have to wait, conversion also takes the filemap_invalidate_lock() that
>> filemap operations use.
>
> I could be convinced otherwise but I'm thinking the overhead of another
> lock for the sake of simplicity is a good trade off.  I don't think any of
> the conversions are a fast path operation are they?

Haha, I think not having another lock is simpler! Looks like it's
starting to get subjective.

For the next RFC, I'll go with re-using the filemap_invalidate_lock(),
and the next RFC will have quite some changes too. Please feel free to
bring this up again. The next RFC is an RFC and won't be committal
anyway :)

>
> Ira
>
> [snip]