Message ID | 20171027073133.24874-1-anand.jain@oracle.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 27.10.2017 10:31, Anand Jain wrote: > btrfs_read_dev_super() returns -1 upon not finding a suitable SB, > change that to return 1 instead, so that it can reserve the < 0 > values for the errno communications. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> I believe this is buggy since it will be masking errors. We call btrfs_read_dev_super in open_ctree_with_broken_chunk if the function returns 1 we eventually exit as: return ERR_PTR(ret); And then perform IS_ERR on the returned value. IS_ERR is essentially: ((x) >= (unsigned long)-MAX_ERRNO) where x could be 1 and MAX_ERRNO is 4095 So you just broke all functions which return the ret of btrfs_read_dev_super as ERR_PTR. NAK > --- > An independent patch, not related to any recent patch sent to ML. > > disk-io.c | 2 +- > utils.c | 2 +- > volumes.c | 2 +- > 3 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/disk-io.c b/disk-io.c > index f5edc4796619..ba87bb4ce6da 100644 > --- a/disk-io.c > +++ b/disk-io.c > @@ -1491,7 +1491,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr, > } > } > > - return transid > 0 ? 0 : -1; > + return transid > 0 ? 0 : 1; > } > > static int write_dev_supers(struct btrfs_fs_info *fs_info, > diff --git a/utils.c b/utils.c > index 524f463d3140..7574cda8151c 100644 > --- a/utils.c > +++ b/utils.c > @@ -1678,7 +1678,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args, > disk_super = (struct btrfs_super_block *)buf; > ret = btrfs_read_dev_super(fd, disk_super, > BTRFS_SUPER_INFO_OFFSET, 0); > - if (ret < 0) { > + if (ret) { > ret = -EIO; > goto out; > } > diff --git a/volumes.c b/volumes.c > index 2209e5a9100b..87008bfdd586 100644 > --- a/volumes.c > +++ b/volumes.c > @@ -268,7 +268,7 @@ int btrfs_scan_one_device(int fd, const char *path, > > disk_super = (struct btrfs_super_block *)buf; > ret = btrfs_read_dev_super(fd, disk_super, super_offset, sbflags); > - if (ret < 0) > + if (ret) > return -EIO; > devid = btrfs_stack_device_id(&disk_super->dev_item); > if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_METADUMP) > -- 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
On 2017年10月27日 15:54, Nikolay Borisov wrote: > > > On 27.10.2017 10:31, Anand Jain wrote: >> btrfs_read_dev_super() returns -1 upon not finding a suitable SB, >> change that to return 1 instead, so that it can reserve the < 0 >> values for the errno communications. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> > > I believe this is buggy since it will be masking errors. We call > btrfs_read_dev_super in open_ctree_with_broken_chunk if the function > returns 1 we eventually exit as: return ERR_PTR(ret); And then perform > IS_ERR on the returned value. IS_ERR is essentially: > ((x) >= (unsigned long)-MAX_ERRNO) where x could be 1 and MAX_ERRNO is 4095 > > So you just broke all functions which return the ret of > btrfs_read_dev_super as ERR_PTR. > > > NAK This doesn't looks good to me either. No suitable superblock is already an error. Why not just returning -ENOENT and let caller who really needs to distinguish this to catch that -ENOENT? Thanks, Qu > >> --- >> An independent patch, not related to any recent patch sent to ML. >> >> disk-io.c | 2 +- >> utils.c | 2 +- >> volumes.c | 2 +- >> 3 files changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/disk-io.c b/disk-io.c >> index f5edc4796619..ba87bb4ce6da 100644 >> --- a/disk-io.c >> +++ b/disk-io.c >> @@ -1491,7 +1491,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr, >> } >> } >> >> - return transid > 0 ? 0 : -1; >> + return transid > 0 ? 0 : 1; >> } >> >> static int write_dev_supers(struct btrfs_fs_info *fs_info, >> diff --git a/utils.c b/utils.c >> index 524f463d3140..7574cda8151c 100644 >> --- a/utils.c >> +++ b/utils.c >> @@ -1678,7 +1678,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args, >> disk_super = (struct btrfs_super_block *)buf; >> ret = btrfs_read_dev_super(fd, disk_super, >> BTRFS_SUPER_INFO_OFFSET, 0); >> - if (ret < 0) { >> + if (ret) { >> ret = -EIO; >> goto out; >> } >> diff --git a/volumes.c b/volumes.c >> index 2209e5a9100b..87008bfdd586 100644 >> --- a/volumes.c >> +++ b/volumes.c >> @@ -268,7 +268,7 @@ int btrfs_scan_one_device(int fd, const char *path, >> >> disk_super = (struct btrfs_super_block *)buf; >> ret = btrfs_read_dev_super(fd, disk_super, super_offset, sbflags); >> - if (ret < 0) >> + if (ret) >> return -EIO; >> devid = btrfs_stack_device_id(&disk_super->dev_item); >> if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_METADUMP) >> > -- > 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 >
On 10/27/2017 04:04 PM, Qu Wenruo wrote: > > > On 2017年10月27日 15:54, Nikolay Borisov wrote: >> >> >> On 27.10.2017 10:31, Anand Jain wrote: >>> btrfs_read_dev_super() returns -1 upon not finding a suitable SB, >>> change that to return 1 instead, so that it can reserve the < 0 >>> values for the errno communications. >>> >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> >> I believe this is buggy since it will be masking errors. We call >> btrfs_read_dev_super in open_ctree_with_broken_chunk if the function >> returns 1 we eventually exit as: return ERR_PTR(ret); And then perform >> IS_ERR on the returned value. IS_ERR is essentially: >> ((x) >= (unsigned long)-MAX_ERRNO) where x could be 1 and MAX_ERRNO is 4095 >> >> So you just broke all functions which return the ret of >> btrfs_read_dev_super as ERR_PTR. >> >> >> NAK > > This doesn't looks good to me either. > > No suitable superblock is already an error. > Why not just returning -ENOENT and let caller who really needs to > distinguish this to catch that -ENOENT? Thanks for pointing out. Will fix it. IMO, -ENOENT is good for now. (Though the actual cleanup would be to.. There are two parts in btrfs_read_dev_super() one to read only the BTRFS_SUPER_INFO_OFFSET SB. And the other to pick the latest SB by generation #. So splitting them two separate functions will let us know where do we use what in a much clearer way. And for 2nd pread64 we should probably use errno.) Thanks, Anand > Thanks, > Qu > >> >>> --- >>> An independent patch, not related to any recent patch sent to ML. >>> >>> disk-io.c | 2 +- >>> utils.c | 2 +- >>> volumes.c | 2 +- >>> 3 files changed, 3 insertions(+), 3 deletions(-) >>> >>> diff --git a/disk-io.c b/disk-io.c >>> index f5edc4796619..ba87bb4ce6da 100644 >>> --- a/disk-io.c >>> +++ b/disk-io.c >>> @@ -1491,7 +1491,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr, >>> } >>> } >>> >>> - return transid > 0 ? 0 : -1; >>> + return transid > 0 ? 0 : 1; >>> } >>> >>> static int write_dev_supers(struct btrfs_fs_info *fs_info, >>> diff --git a/utils.c b/utils.c >>> index 524f463d3140..7574cda8151c 100644 >>> --- a/utils.c >>> +++ b/utils.c >>> @@ -1678,7 +1678,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args, >>> disk_super = (struct btrfs_super_block *)buf; >>> ret = btrfs_read_dev_super(fd, disk_super, >>> BTRFS_SUPER_INFO_OFFSET, 0); >>> - if (ret < 0) { >>> + if (ret) { >>> ret = -EIO; >>> goto out; >>> } >>> diff --git a/volumes.c b/volumes.c >>> index 2209e5a9100b..87008bfdd586 100644 >>> --- a/volumes.c >>> +++ b/volumes.c >>> @@ -268,7 +268,7 @@ int btrfs_scan_one_device(int fd, const char *path, >>> >>> disk_super = (struct btrfs_super_block *)buf; >>> ret = btrfs_read_dev_super(fd, disk_super, super_offset, sbflags); >>> - if (ret < 0) >>> + if (ret) >>> return -EIO; >>> devid = btrfs_stack_device_id(&disk_super->dev_item); >>> if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_METADUMP) >>> >> -- >> 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
diff --git a/disk-io.c b/disk-io.c index f5edc4796619..ba87bb4ce6da 100644 --- a/disk-io.c +++ b/disk-io.c @@ -1491,7 +1491,7 @@ int btrfs_read_dev_super(int fd, struct btrfs_super_block *sb, u64 sb_bytenr, } } - return transid > 0 ? 0 : -1; + return transid > 0 ? 0 : 1; } static int write_dev_supers(struct btrfs_fs_info *fs_info, diff --git a/utils.c b/utils.c index 524f463d3140..7574cda8151c 100644 --- a/utils.c +++ b/utils.c @@ -1678,7 +1678,7 @@ int get_fs_info(const char *path, struct btrfs_ioctl_fs_info_args *fi_args, disk_super = (struct btrfs_super_block *)buf; ret = btrfs_read_dev_super(fd, disk_super, BTRFS_SUPER_INFO_OFFSET, 0); - if (ret < 0) { + if (ret) { ret = -EIO; goto out; } diff --git a/volumes.c b/volumes.c index 2209e5a9100b..87008bfdd586 100644 --- a/volumes.c +++ b/volumes.c @@ -268,7 +268,7 @@ int btrfs_scan_one_device(int fd, const char *path, disk_super = (struct btrfs_super_block *)buf; ret = btrfs_read_dev_super(fd, disk_super, super_offset, sbflags); - if (ret < 0) + if (ret) return -EIO; devid = btrfs_stack_device_id(&disk_super->dev_item); if (btrfs_super_flags(disk_super) & BTRFS_SUPER_FLAG_METADUMP)
btrfs_read_dev_super() returns -1 upon not finding a suitable SB, change that to return 1 instead, so that it can reserve the < 0 values for the errno communications. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- An independent patch, not related to any recent patch sent to ML. disk-io.c | 2 +- utils.c | 2 +- volumes.c | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-)