btrfs_scan_one_device return error code

btrfs_scan_one_device return error code

[Anand Jain]

Hi,

  /dev/sdc does not contain btrfs SB at all..

---
  # btrfs dev scan /dev/sdc
  Scanning for Btrfs filesystems in '/dev/sdc'
  ERROR: unable to scan the device '/dev/sdc' - Invalid argument
---

  here appropriate error is something like
  no btrfs found on dev

  However btrfs_scan_one_device (kernel) returns -EINVAL
  for other errors too

  Does the below fix sound reasonable ?

Thanks, Anand

--------------
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6b9cff4..d6deae0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -808,7 +808,7 @@ int btrfs_scan_one_device(const char *path, fmode_t 
flags, void *holder,
         struct block_device *bdev;
         struct page *page;
         void *p;
-       int ret = -EINVAL;
+       int ret = 0;
         u64 devid;
         u64 transid;
         u64 total_devices;
@@ -833,24 +833,32 @@ int btrfs_scan_one_device(const char *path, 
fmode_t flags, void *holder,
         }

         /* make sure our super fits in the device */
-       if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
+       if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode)) {
+               ret = -ENOSPC;
                 goto error_bdev_put;
+       }

         /* make sure our super fits in the page */
-       if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
+       if (sizeof(*disk_super) > PAGE_CACHE_SIZE) {
+               ret = -ENOMEM;
                 goto error_bdev_put;
+       }

         /* make sure our super doesn't straddle pages on disk */
         index = bytenr >> PAGE_CACHE_SHIFT;
-       if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
+       if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != 
index) {
+               ret = -ENOMEM;
                 goto error_bdev_put;
+       }

         /* pull in the page with our super */
         page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
                                    index, GFP_NOFS);

-       if (IS_ERR_OR_NULL(page))
+       if (IS_ERR_OR_NULL(page)) {
+               ret = -EIO;
                 goto error_bdev_put;
+       }

         p = kmap(page);

@@ -858,8 +866,10 @@ int btrfs_scan_one_device(const char *path, fmode_t 
flags, void *holder,
         disk_super = p + (bytenr & ~PAGE_CACHE_MASK);

         if (btrfs_super_bytenr(disk_super) != bytenr ||
-           disk_super->magic != cpu_to_le64(BTRFS_MAGIC))
+           disk_super->magic != cpu_to_le64(BTRFS_MAGIC)) {
+               ret = -EINVAL;
                 goto error_unmap;
+       }

         devid = btrfs_stack_device_id(&disk_super->dev_item);
         transid = btrfs_super_generation(disk_super);
-------------------

Re: btrfs_scan_one_device return error code

[Eric Sandeen]

On 3/14/13 3:31 AM, Anand Jain wrote:
> 
> Hi,
> 
>  /dev/sdc does not contain btrfs SB at all..
> 
> ---
>  # btrfs dev scan /dev/sdc
>  Scanning for Btrfs filesystems in '/dev/sdc'
>  ERROR: unable to scan the device '/dev/sdc' - Invalid argument
> ---
> 
>  here appropriate error is something like
>  no btrfs found on dev
> 
>  However btrfs_scan_one_device (kernel) returns -EINVAL
>  for other errors too
> 
>  Does the below fix sound reasonable ?

Overall, things like "Insufficient memory" or "No space left on device"
seem like poor fits for problems like "your superblock straddles 2 pages."
or "I didn't find btrfs."  I'm not sure it's any more useful to the user
than "Invalid argument"

If non-btrfs devices aren't scanned as a matter of course, you could
add printks to each failure case.  But I fear that they are scanned
regularly and this might pollute the logs with nonsense & noise.

I guess you are not the first one to realize it, though:

                /*
                 * FIXME: which are the error code returned by this ioctl ?
                 * it seems that is impossible to understand if there no is
                 * a btrfs filesystem from an I/O error !!!
                 */
                ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args);

IO errors could be returned, as could true ENOMEM situations, but
for basically non-btrfs data found on the device, nothing really fits.

It's too bad that ioctl arg doesn't have room to pass back
any other status.

A rev of the ioctl (BTRFS_IOC_SCAN_DEV_V2), which allows
passing status flags back out, would solve the problem nicely
I think.

ret = ioctl(fd, BTRFS_IOC_SCAN_DEV, &args_v2);

if (ret < 0)
	<ioctl itself failed due to EIO, ENOMEM, EFAULT etc>

if (!ret && (args_v2.flags & BTRFS_DEV_NOT_FOUND))
	<ioctl succeeded in finding no btrfs>
	<tell user no btrfs was found>


As a hack you could strncpy the informational error
string into the args.name.  ;)  (no, don't really do that)

-Eric

> Thanks, Anand
> 
> --------------
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6b9cff4..d6deae0 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -808,7 +808,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>         struct block_device *bdev;
>         struct page *page;
>         void *p;
> -       int ret = -EINVAL;
> +       int ret = 0;
>         u64 devid;
>         u64 transid;
>         u64 total_devices;
> @@ -833,24 +833,32 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>         }
> 
>         /* make sure our super fits in the device */
> -       if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
> +       if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode)) {
> +               ret = -ENOSPC;
>                 goto error_bdev_put;
> +       }
> 
>         /* make sure our super fits in the page */
> -       if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
> +       if (sizeof(*disk_super) > PAGE_CACHE_SIZE) {
> +               ret = -ENOMEM;
>                 goto error_bdev_put;
> +       }
> 
>         /* make sure our super doesn't straddle pages on disk */
>         index = bytenr >> PAGE_CACHE_SHIFT;
> -       if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
> +       if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index) {
> +               ret = -ENOMEM;
>                 goto error_bdev_put;
> +       }
> 
>         /* pull in the page with our super */
>         page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
>                                    index, GFP_NOFS);
> 
> -       if (IS_ERR_OR_NULL(page))
> +       if (IS_ERR_OR_NULL(page)) {
> +               ret = -EIO;
>                 goto error_bdev_put;
> +       }
> 
>         p = kmap(page);
> 
> @@ -858,8 +866,10 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>         disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
> 
>         if (btrfs_super_bytenr(disk_super) != bytenr ||
> -           disk_super->magic != cpu_to_le64(BTRFS_MAGIC))
> +           disk_super->magic != cpu_to_le64(BTRFS_MAGIC)) {
> +               ret = -EINVAL;
>                 goto error_unmap;
> +       }
> 
>         devid = btrfs_stack_device_id(&disk_super->dev_item);
>         transid = btrfs_super_generation(disk_super);
> -------------------
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: btrfs_scan_one_device return error code

[Anand Jain]

  As of now a common 'Invalid argument' return code is
  for a set of 5 errors,  that is misleading from
  the end-user context.

  Albeit the proposed return error codes are the closest
  they might not be the perfect though. Since the idea is
  at least if we use 'Invalid argument' only when no
  btrfs SB found, that will solve the bug in this context.

  Any other simpler fix ? or should we go for ioctl arg
  V2 and have room for return error string ? Thanks.

-Anand


>>   /dev/sdc does not contain btrfs SB at all..
>>
>> ---
>>   # btrfs dev scan /dev/sdc
>>   Scanning for Btrfs filesystems in '/dev/sdc'
>>   ERROR: unable to scan the device '/dev/sdc' - Invalid argument
>> ---
>>
>>   here appropriate error is something like
>>   no btrfs found on dev
>>
>>   However btrfs_scan_one_device (kernel) returns -EINVAL
>>   for other errors too
>>
>>   Does the below fix sound reasonable ?



> It's too bad that ioctl arg doesn't have room to pass back
> any other status.
>
> A rev of the ioctl (BTRFS_IOC_SCAN_DEV_V2), which allows
> passing status flags back out, would solve the problem nicely
> I think.


>>
>> --------------
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 6b9cff4..d6deae0 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -808,7 +808,7 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>>          struct block_device *bdev;
>>          struct page *page;
>>          void *p;
>> -       int ret = -EINVAL;
>> +       int ret = 0;
>>          u64 devid;
>>          u64 transid;
>>          u64 total_devices;
>> @@ -833,24 +833,32 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>>          }
>>
>>          /* make sure our super fits in the device */
>> -       if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode))
>> +       if (bytenr + PAGE_CACHE_SIZE >= i_size_read(bdev->bd_inode)) {
>> +               ret = -ENOSPC;
>>                  goto error_bdev_put;
>> +       }
>>
>>          /* make sure our super fits in the page */
>> -       if (sizeof(*disk_super) > PAGE_CACHE_SIZE)
>> +       if (sizeof(*disk_super) > PAGE_CACHE_SIZE) {
>> +               ret = -ENOMEM;
>>                  goto error_bdev_put;
>> +       }
>>
>>          /* make sure our super doesn't straddle pages on disk */
>>          index = bytenr >> PAGE_CACHE_SHIFT;
>> -       if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index)
>> +       if ((bytenr + sizeof(*disk_super) - 1) >> PAGE_CACHE_SHIFT != index) {
>> +               ret = -ENOMEM;
>>                  goto error_bdev_put;
>> +       }
>>
>>          /* pull in the page with our super */
>>          page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
>>                                     index, GFP_NOFS);
>>
>> -       if (IS_ERR_OR_NULL(page))
>> +       if (IS_ERR_OR_NULL(page)) {
>> +               ret = -EIO;
>>                  goto error_bdev_put;
>> +       }
>>
>>          p = kmap(page);
>>
>> @@ -858,8 +866,10 @@ int btrfs_scan_one_device(const char *path, fmode_t flags, void *holder,
>>          disk_super = p + (bytenr & ~PAGE_CACHE_MASK);
>>
>>          if (btrfs_super_bytenr(disk_super) != bytenr ||
>> -           disk_super->magic != cpu_to_le64(BTRFS_MAGIC))
>> +           disk_super->magic != cpu_to_le64(BTRFS_MAGIC)) {
>> +               ret = -EINVAL;
>>                  goto error_unmap;
>> +       }
>>
>>          devid = btrfs_stack_device_id(&disk_super->dev_item);
>>          transid = btrfs_super_generation(disk_super);
>> -------------------