Re: [PATCH] btrfs-progs: Fix get_partition_sector_size_sysfs() to handle loopback and device mapper devices

[Qu Wenruo <quwenruo.btrfs@gmx.com>]

在 2025/8/1 20:33, Zoltan Racz 写道:
> Commit e39ed66 added get_partition_sector_size_sysfs() used by "btrfs device usage"
> which returns the sector size of a partition (or its parent). After more testing
> it turned out it couldn`t handle loopback or mapper devices. This patch adds a fix
> for them.

I can fold this change into the original patch if needed.

Although during my test, even unprivileged users can still do regular 
ioctl based size detection, as long as the user have read permission to 
that device.

And if the user can not even read the device, I'd say the environment is 
set up to intentionally prevent user accesses to that block device.

So I'm not convinced about all the fallback method, especially we're 
doing a lot of special handling (partition vs raw devices).

Mind to also provide the test setup you're using and the involved block 
device mode?

> 
> Signed-off-by: Zoltan Racz <racz.zoli@gmail.com>
> ---
>   common/device-utils.c | 48 +++++++++++++++++++++++++++++--------------
>   1 file changed, 33 insertions(+), 15 deletions(-)
> 
> diff --git a/common/device-utils.c b/common/device-utils.c
> index dd781bc5..a75194bf 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -353,26 +353,44 @@ static ssize_t get_partition_sector_size_sysfs(const char *name)
>   	char sysfs[PATH_MAX] = {};
>   	char sizebuf[128];
>   
> -	snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
> +	/*
> +	 * First we look for hw_sector_size directly directly under
> +	 * /sys/class/block/[partition_name]/queue. In case of loopback and
> +	 * device mapper devices there is no parent device (like /dev/sda1 -> /dev/sda),
> +	 * and the partition`s sysfs folder itself contains informations regarding
> +	 * the sector size
> +	 */
> +	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", name);
> +	sysfd = open(sysfs, O_RDONLY);
>   
> -	if (!realpath(link_path, real_path)) {
> -		error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> -		return -1;
> -	}
> +	if (sysfd < 0) {

Just a small nitpic, it's better to check the errno against ENOENT.

But my question still stands, does it really make sense to use sysfs as 
a fallback?

Thanks,
Qu

> +		/*
> +		 * If we couldn`t find it, it means our partition is created on a real
> +		 * device and we need to find its parent
> +		 */
> +		snprintf(link_path, PATH_MAX, "/sys/class/block/%s/..", name);
>   
> -	dev_name = basename(real_path);
> +		if (!realpath(link_path, real_path)) {
> +			error("Failed to resolve realpath of %s: %s\n", link_path, strerror(errno));
> +			return -1;
> +		}
>   
> -	if (!dev_name) {
> -		error("Failed to determine basename for path %s\n", real_path);
> -		return -1;
> -	}
> +		dev_name = basename(real_path);
>   
> -	snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> +		if (!dev_name) {
> +			error("Failed to determine basename for path %s\n", real_path);
> +			return -1;
> +		}
>   
> -	sysfd = open(sysfs, O_RDONLY);
> -	if (sysfd < 0) {
> -		error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> -		return -1;
> +		memset(sysfs, 0, PATH_MAX);
> +		snprintf(sysfs, PATH_MAX, "/sys/class/block/%s/queue/hw_sector_size", dev_name);
> +
> +		sysfd = open(sysfs, O_RDONLY);
> +
> +		if (sysfd < 0) {
> +			error("Error opening %s to determine dev sector size: %s\n", real_path, strerror(errno));
> +			return -1;
> +		}
>   	}
>   
>   	ret = sysfs_read_file(sysfd, sizebuf, sizeof(sizebuf));