On Wed, Apr 15, 2020 at 03:53:46PM +0300, Nikolay Borisov wrote:
> Instead of returning both the page and the super block structure, make
> btrfs_read_disk_super just return a pointer to struct btrfs_disk_super.
> As a result the function signature is simplified. Also,
> read_cache_page_gfp can never return NULL so check its return value only
> for IS_ERR.
^^^^^^^^^^^^^
> /* pull in the page with our super */
> - *page = read_cache_page_gfp(bdev->bd_inode->i_mapping,
> - index, GFP_KERNEL);
> + page = read_cache_page_gfp(bdev->bd_inode->i_mapping, index,
> + GFP_KERNEL);
>
> - if (IS_ERR(*page))
> - return 1;
> + if (IS_ERR(page))
> + return ERR_CAST(page);
There's no NULL check as mentioned in the changelog, or do you mean to
make it explicit that there's no NULL check? The docs of
read_cache_page_gfp say it returns ERR_PTR so I consider it clear
enough. The changelog wording confuses me a bit, but otherwise the patch
looks good.
> - p = page_address(*page);
> + p = page_address(page);
>
> /* align our pointer to the offset of the super block */
> - *disk_super = p + offset_in_page(bytenr);
> + disk_super = p + offset_in_page(bytenr);
>
> - if (btrfs_super_bytenr(*disk_super) != bytenr ||
> - btrfs_super_magic(*disk_super) != BTRFS_MAGIC) {
> + if (btrfs_super_bytenr(disk_super) != bytenr ||
> + btrfs_super_magic(disk_super) != BTRFS_MAGIC) {
> btrfs_release_disk_super(p);
> - return 1;
> + return ERR_PTR(-EUCLEAN);
> }
>
> - if ((*disk_super)->label[0] &&
> - (*disk_super)->label[BTRFS_LABEL_SIZE - 1])
> - (*disk_super)->label[BTRFS_LABEL_SIZE - 1] = '\0';
> + if (disk_super->label[0] && disk_super->label[BTRFS_LABEL_SIZE - 1])
> + disk_super->label[BTRFS_LABEL_SIZE - 1] = '\0';
>
> - return 0;
> + return disk_super;
> }
>
> int btrfs_forget_devices(const char *path)
> @@ -1319,7 +1319,6 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
> bool new_device_added = false;
> struct btrfs_device *device = NULL;
> struct block_device *bdev;
> - struct page *page;
> u64 bytenr;
>
> lockdep_assert_held(&uuid_mutex);
> @@ -1337,7 +1336,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, fmode_t flags,
> if (IS_ERR(bdev))
> return ERR_CAST(bdev);
>
> - if (btrfs_read_disk_super(bdev, bytenr, &page, &disk_super)) {
> + disk_super = btrfs_read_disk_super(bdev, bytenr);
> + if (IS_ERR(disk_super)) {
> device = ERR_PTR(-EINVAL);
> goto error_bdev_put;
> }
> --
> 2.17.1
