Re: [PATCH 1/1] loop: sync filesystem cache before getting file size in get_size()

[Yu Kuai <yukuai1@huaweicloud.com>]

Hi,

在 2025/08/09 2:50, Mishra, Rajeev 写道:
> Thanks, Kuai, for the quick review — I really appreciate it. Please let me know if you have any questions or if I missed addressing anything.
> My response to queries is inline below.
> Rajeev
> 
> From: Yu Kuai <yukuai1@huaweicloud.com>
> Date: Thursday, August 7, 2025 at 9:48 PM
> To: Mishra, Rajeev <rajeevm@hpe.com>, axboe@kernel.dk <axboe@kernel.dk>
> Cc: linux-block@vger.kernel.org <linux-block@vger.kernel.org>, linux-kernel@vger.kernel.org <linux-kernel@vger.kernel.org>, yukuai (C) <yukuai3@huawei.com>
> Subject: Re: [PATCH 1/1] loop: sync filesystem cache before getting file size in get_size()
> Hi,
> 
> 在 2025/08/08 7:25, Rajeev Mishra 写道:
>> The get_size() function now uses vfs_getattr_nosec() with AT_STATX_SYNC_AS_STAT
> 
> With a quick code review, I didn't found how can that flag ensure
> filesystem cache is synchronized, can you explain in detail? Or Do you
> mean getattr for filesystem like fuse to get latest data from server?
> Response ---
>>> Thanks for the quick review. The AT_STATX_SYNC_AS_STAT flag tells
>>> the VFS layer to synchronize cached metadata before returning file attributes.
>>> This is particularly important for distributed/network filesystems where
>>> the local cache may not reflect the current file size on the server.
> ---

I think this is the default behavior, unless AT_STATX_DONT_SYNC is set.
So I think you don't need to emphasize the flag AT_STATX_SYNC_AS_STAT,
otherwise, at least I will think if the flag is not set, cache won't be
synchronized.

> 
>> to ensure filesystem cache is synchronized before retrieving file size. This
>> provides more accurate size information, especially when:
>>
>> - The backing file size has been changed by another process
>> - The file is on a network filesystem (NFS, CIFS, etc.)
>> - The file is being modified concurrently
> 
> I don't think this make sense in real world. If a file is already used
> by loop device, then user should avoid modifying this file directly. For
> a file in fuse, I feel it's not good to use it as loop device.
> Response---
>>> I encountered this issue specifically with Lustre filesystem during testing
>>> I did following --
>>> 1. File was created on Lustre
>>> 2. dd was done to write data on the file
>>> 3. ls confirmed the size
>>> 4. Loop device setup was done on the file immediately
>>> 5. write was issued with less space
>>> 6.above happened as file size was not correctly captured by loop device
> 
>>> I agree that network/distributed filesystems aren't ideal for loop devices,
>>> but they are used in practice (container images on shared storage, diskless
>>> systems, etc.). The fallback to i_size_read() ensures no performance penalty
>>> for local filesystems while improving reliability for network filesystems.

Ok, I agree with this case, can you please just keep the network
filesystem, and remove the description about concurrent modifying of
loop file?

> ____
> 
>> - The most accurate size is needed for loop device setup
>>> The implementation gracefully falls back to i_size_read() if
> vfs_getattr_nosec()
>> fails, maintaining backward compatibility.
> Response –
> ___
>>>> you mean using the flag sync has backward compatibility issue ? or using function itself ?

Nevermind, this is from your orginal patch, I somehow added a new line
break.
> ___
> 
>>>
>> Signed-off-by: Rajeev Mishra <rajeevm@hpe.com>
>> ---
>>    drivers/block/loop.c | 31 +++++++++++++++++++++++++++++--
>>    1 file changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index 1b6ee91f8eb9..15d5edbc69ce 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -137,12 +137,39 @@ static void loop_global_unlock(struct loop_device *lo, bool global)
>>    static int max_part;
>>    static int part_shift;
>>
>> +/**
>> + * get_size - calculate the effective size of a loop device
>> + * @offset: offset into the backing file
>> + * @sizelimit: user-specified size limit
>> + * @file: the backing file
>> + *
>> + * Calculate the effective size of the loop device
>> + *
>> + * Returns: size in 512-byte sectors, or 0 if invalid
>> + */
>>    static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>>    {
>> +     struct kstat stat;
>>         loff_t loopsize;
>> +     int ret;
>> +
>> +     /*
>> +      * Get file attributes for validation. We use vfs_getattr() to ensure
>> +      * we have up-to-date file size information.
>> +      */
>> +     ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE,
>> +                             AT_STATX_SYNC_AS_STAT);
>> +     if (ret) {
>> +             /*
>> +              * If we can't get attributes, fall back to i_size_read()
>> +              * which should work for most cases.
>> +              */
>> +             loopsize = i_size_read(file->f_mapping->host);
>> +     } else {
>> +             /* Use the size from getattr for consistency */
>> +             loopsize = stat.size;
>> +     }
> 
> I'm ok switch from i_size_read() to getattr, however, the commit message
> is confusing for me :(
> 
> Response --
>>> I will make the commit message clear and simple. Just wanted to understand
>>> if using this will be good “vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);”
>>> is good I will replace i_size_read() with above code
>>> do let me know if this will have any backward compatibility issue
>>> Thanks again for your help

I'm good, and i_size_read() is not needed anymore.

Thanks,
Kuai

> 
> 
> Thanks,
> Kuai
>>
>> -     /* Compute loopsize in bytes */
>> -     loopsize = i_size_read(file->f_mapping->host);
>>         if (offset > 0)
>>                 loopsize -= offset;
>>         /* offset is beyond i_size, weird but possible */
>>