[PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Rajeev Mishra]

The get_size() function now uses vfs_getattr_nosec() instead of
i_size_read() to obtain file size information. This provides more
accurate results for network filesystems where cached metadata
may be stale, ensuring the loop device reflects the current file
size rather than potentially outdated cached values.

Signed-off-by: Rajeev Mishra <rajeevm@hpe.com>
---
 drivers/block/loop.c | 24 ++++++++++++++++++++++--
 1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 1b6ee91f8eb9..c418c47db76e 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -137,12 +137,32 @@ 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 the accurate file size. This will prevent caching
+	 * issue that occurs at filesystem layer.
+	 */
+	ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);
+	if (ret)
+		return 0;
+
+	loopsize = stat.size;
 
-	/* 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 */
-- 
2.43.7

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Damien Le Moal]

On 8/12/25 4:03 AM, Rajeev Mishra wrote:
> The get_size() function now uses vfs_getattr_nosec() instead of
> i_size_read() to obtain file size information. This provides more
> accurate results for network filesystems where cached metadata
> may be stale, ensuring the loop device reflects the current file
> size rather than potentially outdated cached values.
> 
> Signed-off-by: Rajeev Mishra <rajeevm@hpe.com>
> ---
>  drivers/block/loop.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 1b6ee91f8eb9..c418c47db76e 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -137,12 +137,32 @@ 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 the accurate file size. This will prevent caching
> +	 * issue that occurs at filesystem layer.
> +	 */
> +	ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);
> +	if (ret)
> +		return 0;

return 0 here is odd. Why not "return ret;" to propagate the error if any ?
An error may come from the underlying FS inode->i_op->getattr().

> +
> +	loopsize = stat.size;
>  
> -	/* 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 */


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Yu Kuai]

Hi,

在 2025/08/12 9:40, Damien Le Moal 写道:
> On 8/12/25 4:03 AM, Rajeev Mishra wrote:
>> The get_size() function now uses vfs_getattr_nosec() instead of
>> i_size_read() to obtain file size information. This provides more
>> accurate results for network filesystems where cached metadata
>> may be stale, ensuring the loop device reflects the current file
>> size rather than potentially outdated cached values.
>>
>> Signed-off-by: Rajeev Mishra <rajeevm@hpe.com>
>> ---
>>   drivers/block/loop.c | 24 ++++++++++++++++++++++--
>>   1 file changed, 22 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>> index 1b6ee91f8eb9..c418c47db76e 100644
>> --- a/drivers/block/loop.c
>> +++ b/drivers/block/loop.c
>> @@ -137,12 +137,32 @@ 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 the accurate file size. This will prevent caching
>> +	 * issue that occurs at filesystem layer.
>> +	 */
>> +	ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);
>> +	if (ret)
>> +		return 0;
> 
> return 0 here is odd. Why not "return ret;" to propagate the error if any ?
> An error may come from the underlying FS inode->i_op->getattr().

This helper is supposed to return the size, all the callers don't
hanldle error value for now.

However, I agree return error value instead of set disk size to 0 will
make more sense, at least from ioctl path. Perhaps go through all the
callers and see, if we want to handle errors or set disk size to 0.

Thanks,
Kuai

> 
>> +
>> +	loopsize = stat.size;
>>   
>> -	/* 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 */
> 
> 

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Rajeev Mishra]

Hi Kuai,

Thank you for the feedback on the v2 patch regarding error handling.

Yu mentioned:
> return 0 here is odd. Why not "return ret;" to propagate the error if any ?

I understand the concern about proper error propagation. However, there's a 
type compatibility issue I'd like to discuss before implementing v3:

1. Current function signature: `static loff_t get_size(...)` 
   - Returns size as positive loff_t (unsigned 64-bit)  
   - All callers expect non-negative size values

2. vfs_getattr_nosec() error codes are negative integers (-ENOENT, -EIO, etc.)
   - Returning `ret` would cast negative errors to huge positive numbers
   - This could cause loop devices to appear as exabyte-sized

3. Current callers like loop_set_size() don't handle error checking

Would you prefer for v3:
a) Change function signature to `int get_size(..., loff_t *size)` and update all callers  
b) Different approach?

diff with ret approach

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index c418c47db76e..15117630c6c1 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -142,12 +142,13 @@ static int part_shift;
  * @offset: offset into the backing file
  * @sizelimit: user-specified size limit
  * @file: the backing file
+ * @size: pointer to store the calculated size
  *
  * Calculate the effective size of the loop device
  *
- * Returns: size in 512-byte sectors, or 0 if invalid
+ * Returns: 0 on success, negative error code on failure
  */
-static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
+static int get_size(loff_t offset, loff_t sizelimit, struct file *file, loff_t *size)
 {
        struct kstat stat;
        loff_t loopsize;
@@ -159,7 +160,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
         */
        ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);
        if (ret)
-               return 0;
+               return ret;

        loopsize = stat.size;

@@ -167,7 +168,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
                loopsize -= offset;
        /* offset is beyond i_size, weird but possible */
        if (loopsize < 0)
-               return 0;
+               return -EINVAL;

        if (sizelimit > 0 && sizelimit < loopsize)
                loopsize = sizelimit;
@@ -175,12 +176,20 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
         * Unfortunately, if we want to do I/O on the device,
         * the number of 512-byte sectors has to fit into a sector_t.
         */
-       return loopsize >> 9;
+       *size = loopsize >> 9;
+       return 0;
 }

 static loff_t get_loop_size(struct loop_device *lo, struct file *file)
 {
-       return get_size(lo->lo_offset, lo->lo_sizelimit, file);
+       loff_t size;
+       int ret;
+
+       ret = get_size(lo->lo_offset, lo->lo_sizelimit, file, &size);
+       if (ret)
+               return 0;  /* Fallback to 0 on error for backward compatibility */
+
+       return size;
 }


I am happy to implement whichever direction you think is best.

Thanks,
Rajeev

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Damien Le Moal]

On 8/12/25 12:32 PM, Rajeev Mishra wrote:
> Hi Kuai,
> 
> Thank you for the feedback on the v2 patch regarding error handling.
> 
> Yu mentioned:
>> return 0 here is odd. Why not "return ret;" to propagate the error if any ?
> 
> I understand the concern about proper error propagation. However, there's a 
> type compatibility issue I'd like to discuss before implementing v3:
> 
> 1. Current function signature: `static loff_t get_size(...)` 
>    - Returns size as positive loff_t (unsigned 64-bit)  
>    - All callers expect non-negative size values
> 
> 2. vfs_getattr_nosec() error codes are negative integers (-ENOENT, -EIO, etc.)
>    - Returning `ret` would cast negative errors to huge positive numbers
>    - This could cause loop devices to appear as exabyte-sized
> 
> 3. Current callers like loop_set_size() don't handle error checking
> 
> Would you prefer for v3:
> a) Change function signature to `int get_size(..., loff_t *size)` and update all callers  
> b) Different approach?
> 
> diff with ret approach
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index c418c47db76e..15117630c6c1 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -142,12 +142,13 @@ static int part_shift;
>   * @offset: offset into the backing file
>   * @sizelimit: user-specified size limit
>   * @file: the backing file
> + * @size: pointer to store the calculated size
>   *
>   * Calculate the effective size of the loop device
>   *
> - * Returns: size in 512-byte sectors, or 0 if invalid
> + * Returns: 0 on success, negative error code on failure
>   */
> -static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)

Since loff_t is "long long", so a signed type, I would keep this interface and
add a negative error check in the 2 call sites for get_size(). That is simpler.

> +static int get_size(loff_t offset, loff_t sizelimit, struct file *file, loff_t *size)
>  {
>         struct kstat stat;
>         loff_t loopsize;
> @@ -159,7 +160,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>          */
>         ret = vfs_getattr_nosec(&file->f_path, &stat, STATX_SIZE, 0);
>         if (ret)
> -               return 0;
> +               return ret;
> 
>         loopsize = stat.size;
> 
> @@ -167,7 +168,7 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>                 loopsize -= offset;
>         /* offset is beyond i_size, weird but possible */
>         if (loopsize < 0)
> -               return 0;
> +               return -EINVAL;
> 
>         if (sizelimit > 0 && sizelimit < loopsize)
>                 loopsize = sizelimit;
> @@ -175,12 +176,20 @@ static loff_t get_size(loff_t offset, loff_t sizelimit, struct file *file)
>          * Unfortunately, if we want to do I/O on the device,
>          * the number of 512-byte sectors has to fit into a sector_t.
>          */
> -       return loopsize >> 9;
> +       *size = loopsize >> 9;
> +       return 0;
>  }
> 
>  static loff_t get_loop_size(struct loop_device *lo, struct file *file)
>  {
> -       return get_size(lo->lo_offset, lo->lo_sizelimit, file);
> +       loff_t size;
> +       int ret;
> +
> +       ret = get_size(lo->lo_offset, lo->lo_sizelimit, file, &size);
> +       if (ret)
> +               return 0;  /* Fallback to 0 on error for backward compatibility */
> +
> +       return size;
>  }
> 
> 
> I am happy to implement whichever direction you think is best.
> 
> Thanks,
> Rajeev


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Al Viro]

On Tue, Aug 12, 2025 at 12:42:44PM +0900, Damien Le Moal wrote:

> Since loff_t is "long long", so a signed type, I would keep this interface and
> add a negative error check in the 2 call sites for get_size(). That is simpler.

Umm...  First of all, what's the point of separate get_size() and loop_get_size()?
Another thing to watch out for - replacing file needs to be careful, lest you
replace the old file that has come to fail vfs_getattr() with new one that
does the same thing ;-)

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Damien Le Moal]

On 8/12/25 1:28 PM, Al Viro wrote:
> On Tue, Aug 12, 2025 at 12:42:44PM +0900, Damien Le Moal wrote:
> 
>> Since loff_t is "long long", so a signed type, I would keep this interface and
>> add a negative error check in the 2 call sites for get_size(). That is simpler.
> 
> Umm...  First of all, what's the point of separate get_size() and loop_get_size()?
> Another thing to watch out for - replacing file needs to be careful, lest you
> replace the old file that has come to fail vfs_getattr() with new one that
> does the same thing ;-)

I did a quick grep and missed the fact that get_size() is mostly used through
get_loop_size(). So yes, making these 2 a single function will be clearer.

And indeed, that:

	/* size of the new backing store needs to be the same */
        if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
                goto out_err;

Will need some massaging.


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Christoph Hellwig]

On Tue, Aug 12, 2025 at 02:17:01PM +0900, Damien Le Moal wrote:
> And indeed, that:
> 
> 	/* size of the new backing store needs to be the same */
>         if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
>                 goto out_err;
> 
> Will need some massaging.

Why?  get_loop_size just derives the first arguments to get_size
from the passed in loop device in the same way the only other caller
to get_size does.  So we can just:

  1) convert loop_set_status to use get_loop_size
  2) Fold get_size into get_loop_size
  3) Maye rename get_size to lo_calculate_size to have a descriptive
     name while we're touching it?
  4) switch to vfs_getattr

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Yu Kuai]

Hi,

在 2025/08/12 16:37, Christoph Hellwig 写道:
> On Tue, Aug 12, 2025 at 02:17:01PM +0900, Damien Le Moal wrote:
>> And indeed, that:
>>
>> 	/* size of the new backing store needs to be the same */
>>          if (get_loop_size(lo, file) != get_loop_size(lo, old_file))
>>                  goto out_err;
>>
>> Will need some massaging.
> 
> Why?  get_loop_size just derives the first arguments to get_size
> from the passed in loop device in the same way the only other caller
> to get_size does.  So we can just:
> 
>    1) convert loop_set_status to use get_loop_size
>    2) Fold get_size into get_loop_size
>    3) Maye rename get_size to lo_calculate_size to have a descriptive
>       name while we're touching it?
>    4) switch to vfs_getattr

This looks good, it's better to refactor a bit before switch to getattr,
and I agree still return 0 on failue is fine.

Thanks,
Kuai

> 
> 
> .
> 

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Christoph Hellwig]

On Tue, Aug 12, 2025 at 12:42:44PM +0900, Damien Le Moal wrote:
> Since loff_t is "long long", so a signed type, I would keep this interface and
> add a negative error check in the 2 call sites for get_size(). That is simpler.

Not ethat the existing code already treats 0 as error for the
offset smaller than size case.  I'm not arguing against and error
return, but that would require auditing all the call chains.  And I'd
rather not multiplex it into the loff_t as that means people will forget
to handle it way too easily.

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Al Viro]

On Tue, Aug 12, 2025 at 03:32:01AM +0000, Rajeev Mishra wrote:
> Hi Kuai,
> 
> Thank you for the feedback on the v2 patch regarding error handling.
> 
> Yu mentioned:
> > return 0 here is odd. Why not "return ret;" to propagate the error if any ?
> 
> I understand the concern about proper error propagation. However, there's a 
> type compatibility issue I'd like to discuss before implementing v3:
> 
> 1. Current function signature: `static loff_t get_size(...)` 
>    - Returns size as positive loff_t (unsigned 64-bit)  
>    - All callers expect non-negative size values
> 
> 2. vfs_getattr_nosec() error codes are negative integers (-ENOENT, -EIO, etc.)
>    - Returning `ret` would cast negative errors to huge positive numbers

Huh? loff_t is signed; had always been that way...

> 3. Current callers like loop_set_size() don't handle error checking

If you start returning errors, they ought to.  Incidentally, it might make
sense to return the size in bytes - just move the shift into loop_set_size()...

Re: [PATCH v2] loop: use vfs_getattr_nosec() for accurate file size

[Christoph Hellwig]

> +/**
> + * 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
> + */

A kerneldoc comment for an internal helper is still a bad idea.