| Message ID | 51418AD5.6090207@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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); >> ------------------- -- 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
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;
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 -------------- 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