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