Pratyush Yadav <[email protected]> writes:
> On Tue, Jun 23 2026, [email protected] wrote:
>
>> Ackerley Tng <[email protected]> writes:
>>
>>> Tarun Sahu <[email protected]> writes:
>>>
>>>> static long kvm_gmem_fallocate(struct file *file, int mode, loff_t offset,
>>>> loff_t len)
>>>> {
>>>> + struct inode *inode = file_inode(file);
>>>> int ret;
>>>> + int idx;
>>>>
>>>> - if (!(mode & FALLOC_FL_KEEP_SIZE))
>>>> - return -EOPNOTSUPP;
>>>> + idx = srcu_read_lock(&kvm_gmem_freeze_srcu);
>>>> + if (kvm_gmem_is_frozen(inode)) {
>>>> + srcu_read_unlock(&kvm_gmem_freeze_srcu, idx);
>>>> + return -EPERM;
>>>> + }
>>>
>>> fallocate may eventually go to kvm_gmem_get_folio(), so that would check
>>> kvm_gmem_is_frozen() twice. Is this meant to catch the punch hole case?
>
> Yeah, I reckon you can get away with doing this check only in
> kvm_gmem_get_folio(). Normally you'd like to fail early, but as of now I
> don't see much of a problem. If you drop the check here and fail in
> kvm_gmem_get_folio() you'd end up taking and releasing the mapping
> invalidate_lock, but this isn't a fast path anyway so I don't think it
> should matter much.
No, Don't agree.
kvm_gmem_get_folios already have the is_frozen check. which blocks the
kvm_gmem_allocate. But not kvm_gmem_punch_hole. Your argument is correct
for kvm_gmem_allocate only. So is_frozen check in fallocate is to
block the punch hole as well. What ackerley said is correct.
>
> I think either way can work just as fine...
>
>>>
>>>>
>>>> - if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
>>>> - return -EOPNOTSUPP;
>>>> + if (!(mode & FALLOC_FL_KEEP_SIZE)) {
>>>> + ret = -EOPNOTSUPP;
>>>> + goto out;
>>>> + }
>>>>
>>>> - if (!PAGE_ALIGNED(offset) || !PAGE_ALIGNED(len))
>>>> - return -EINVAL;
>>>> + if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE)) {
>>>> + ret = -EOPNOTSUPP;
>>>> + goto out;
>>>> + }
>>>> +
>>>> + if (!PAGE_ALIGNED(offset) || !PAGE_ALIGNED(len)) {
>>>> + ret = -EINVAL;
>>>> + goto out;
>>>> + }
>>>
>>> There's some reordering here. Why not let the validation happen like
>>> before, then check kvm_gmem_is_frozen()?
>
> There is no reordering, if I am reading the diff correctly. The diff is
> somewhat misleading. The kvm_gmem_is_frozen() call is added at the top
> of the function, and then all the later checks are in the same place but
> get a goto out (and hence a full body to the if block). So the diff
> reads like reordering, but there is none.
>
That is right. I thought Ackerley was asking why not add the check after
the all the if condition and before the kvm_gmem_punch_hole/allocate calls.
> It would be very neat if scru had a cleanup.h style scope-based locking
> function, but on a quick glance I can't see one.
>
>>
>> To align with design. "stop the fallocate call if inode is frozen, No
>> need to go further". I dont have strict opinion on this. I am fine with
>> taking it across punch hole as well to make it more fine grained. But it
>> will no longer claims stop the fallocate call (allocation one is stopped
>> in separate path: fault path) , though functionally it does the same
>> thing.
>>
>> WDYT?
>>
>> ~Tarun
>
> --
> Regards,
> Pratyush Yadav
