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
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.