Re: [PATCH 2/5] btrfs-progs: add error handling for device_get_partition_size()

[Qu Wenruo <wqu@suse.com>]

在 2025/8/6 04:38, Boris Burkov 写道:
> On Sat, Aug 02, 2025 at 04:25:48PM +0930, Qu Wenruo wrote:
>> The function device_get_partition_size() has all kinds of error paths,
>> but it has no way to return error other than returning 0.
>>
>> This is not helpful for end users to know what's going wrong.
>>
>> Change that function to have a dedicated return value for error
>> handling, and pass a u64 pointer for the result.
> 
> All the callers check ret < 0, rather than ret != 0, so any reason not
> to simply return the positive value in case of no error? Signed
> overflow?
> 
> In the kernel, BLK_DEV_MAX_SECTORS is set to LLONG_MAX >> 9, which I
> think means we are probably OK?

This sounds reasonable, I was under the impression that we need to 
preserve the full U64_MAX for block devices due to our on-disk format.

But if kernel has extra limits, I'm more than happier to use s64.

Thanks,
Qu

> 
>>
>> For most callers they are able to exit gracefully with a proper error
>> message.
>>
>> But for load_device_info(), we allow a failed size probing to continue
>> without 0 size, just as the old code.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   cmds/filesystem-usage.c |  9 +++++++--
>>   cmds/replace.c          | 14 ++++++++++++--
>>   common/device-utils.c   | 29 +++++++++++++++--------------
>>   common/device-utils.h   |  2 +-
>>   4 files changed, 35 insertions(+), 19 deletions(-)
>>
>> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
>> index f812af13482e..e367bffc3a01 100644
>> --- a/cmds/filesystem-usage.c
>> +++ b/cmds/filesystem-usage.c
>> @@ -820,8 +820,13 @@ static int load_device_info(int fd, struct array *devinfos)
>>   			strcpy(info->path, "missing");
>>   		} else {
>>   			strcpy(info->path, (char *)dev_info.path);
>> -			info->device_size =
>> -				device_get_partition_size((const char *)dev_info.path);
>> +			ret = device_get_partition_size((const char *)dev_info.path,
>> +							&info->device_size);
>> +			if (ret < 0) {
>> +				errno = -ret;
>> +				warning("failed to get device size for devid %llu: %m", dev_info.devid);
>> +				info->device_size = 0;
>> +			}
>>   		}
>>   		info->size = dev_info.total_bytes;
>>   		ndevs++;
>> diff --git a/cmds/replace.c b/cmds/replace.c
>> index 887c3251a725..d5b0b0e02ce1 100644
>> --- a/cmds/replace.c
>> +++ b/cmds/replace.c
>> @@ -269,7 +269,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>>   		strncpy_null((char *)start_args.start.srcdev_name, srcdev,
>>   			     BTRFS_DEVICE_PATH_NAME_MAX + 1);
>>   		start_args.start.srcdevid = 0;
>> -		srcdev_size = device_get_partition_size(srcdev);
>> +		ret = device_get_partition_size(srcdev, &srcdev_size);
>> +		if (ret < 0) {
>> +			errno = -ret;
>> +			error("failed to get device size for %s: %m", srcdev);
>> +			goto leave_with_error;
>> +		}
>>   	} else {
>>   		error("source device must be a block device or a devid");
>>   		goto leave_with_error;
>> @@ -279,7 +284,12 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>>   	if (ret)
>>   		goto leave_with_error;
>>   
>> -	dstdev_size = device_get_partition_size(dstdev);
>> +	ret = device_get_partition_size(dstdev, &dstdev_size);
>> +	if (ret < 0) {
>> +		errno = -ret;
>> +		error("failed to get device size for %s: %m", dstdev);
>> +		goto leave_with_error;
>> +	}
>>   	if (srcdev_size > dstdev_size) {
>>   		error("target device smaller than source device (required %llu bytes)",
>>   			srcdev_size);
>> diff --git a/common/device-utils.c b/common/device-utils.c
>> index 783d79555446..6d89d16b029d 100644
>> --- a/common/device-utils.c
>> +++ b/common/device-utils.c
>> @@ -342,7 +342,7 @@ u64 device_get_partition_size_fd(int fd)
>>   	return result;
>>   }
>>   
>> -static u64 device_get_partition_size_sysfs(const char *dev)
>> +static int device_get_partition_size_sysfs(const char *dev, u64 *result_ret)
>>   {
>>   	int ret;
>>   	char path[PATH_MAX] = {};
>> @@ -354,45 +354,46 @@ static u64 device_get_partition_size_sysfs(const char *dev)
>>   
>>   	name = realpath(dev, path);
>>   	if (!name)
>> -		return 0;
>> +		return -errno;
>>   	name = path_basename(path);
>>   
>>   	ret = path_cat3_out(sysfs, "/sys/class/block", name, "size");
>>   	if (ret < 0)
>> -		return 0;
>> +		return ret;
>>   	sysfd = open(sysfs, O_RDONLY);
>>   	if (sysfd < 0)
>> -		return 0;
>> +		return -errno;
>>   	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));
>>   	if (ret < 0) {
>>   		close(sysfd);
>> -		return 0;
>> +		return ret;
>>   	}
>>   	errno = 0;
>>   	size = strtoull(sizebuf, NULL, 10);
>>   	if (size == ULLONG_MAX && errno == ERANGE) {
>>   		close(sysfd);
>> -		return 0;
>> +		return -errno;
>>   	}
>>   	close(sysfd);
>> -	return size;
>> +	*result_ret = size;
>> +	return 0;
>>   }
>>   
>> -u64 device_get_partition_size(const char *dev)
>> +int device_get_partition_size(const char *dev, u64 *result_ret)
>>   {
>> -	u64 result;
>>   	int fd = open(dev, O_RDONLY);
>>   
>>   	if (fd < 0)
>> -		return device_get_partition_size_sysfs(dev);
>> +		return device_get_partition_size_sysfs(dev, result_ret);
>> +
>> +	if (ioctl(fd, BLKGETSIZE64, result_ret) < 0) {
>> +		int ret = -errno;
>>   
>> -	if (ioctl(fd, BLKGETSIZE64, &result) < 0) {
>>   		close(fd);
>> -		return 0;
>> +		return ret;
>>   	}
>>   	close(fd);
>> -
>> -	return result;
>> +	return 0;
>>   }
>>   
>>   /*
>> diff --git a/common/device-utils.h b/common/device-utils.h
>> index cef9405f3a9a..bf04eb0f2fdd 100644
>> --- a/common/device-utils.h
>> +++ b/common/device-utils.h
>> @@ -42,7 +42,7 @@ enum {
>>    */
>>   int device_discard_blocks(int fd, u64 start, u64 len);
>>   int device_zero_blocks(int fd, off_t start, size_t len, const bool direct);
>> -u64 device_get_partition_size(const char *dev);
>> +int device_get_partition_size(const char *dev, u64 *result_ret);
>>   u64 device_get_partition_size_fd(int fd);
>>   u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
>>   int device_get_queue_param(const char *file, const char *param, char *buf, size_t len);
>> -- 
>> 2.50.1
>>