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.
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.
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
