[PATCH v2 0/6] btrfs-progs: device_get_partition_size*() enhancement and cleanup

[PATCH v2 0/6] btrfs-progs: device_get_partition_size*() enhancement and cleanup

[Qu Wenruo]

[CHANGELOG]
v2:
- Include and update Zoltan's fix to device_get_partition_size_sysfs()
  The "/dev/block/<device>/size" attribute is always in sector unit
  (512Bytes), independent from the physical device's block size.

  So the fix is pretty straightforward, just do the left shift before
  returning.

  And update the commit message to properly reflect that.

- Use s64 as the return value of device_get_partition_size*()
  Kernel ensures the max sector number of a device is LLONG_MAX >> 9,
  so s64 is more than enough to cover the device size, and we are safe
  to use the minus part to pass errors.

- Extra error handling when device_get_partition_size*() failed
  Ouput an error message and exit.
  Most callers are already doing that correctly but some are not.

- Keep the sysfs method as the fallback way to grab device size
  Since the device_get_partition_size_sysfs() can be easily fixed, no
  need to use complex path search for sector size.

- Add a new cleanup to remove is_vol_small()

Zoltan recently sent a fix to solve the bug in
device_get_partition_size_sysfs(), which is causing problems for "btrfs
dev usage".

It turns out that, the bug is just the attribute
"/sys/block/<device>/size" is in sector unit (512B), not in bytes.

So fix the bug first by reporting the correct size in bytes.

Then remove several unused functions exposed during the error handling
cleanup.

Finally cleanup device_get_partition_size*() helpers to provide a proper
error handling, not just return 0 for errors, but proper minus erro
code, and extra overflow check.

Qu Wenruo (5):
  btrfs-progs: remove the unnecessary device_get_partition_size() call
  btrfs-progs: remove device_get_partition_size_fd()
  btrfs-progs: remove is_vol_small() helper
  btrfs-progs: add error handling for device_get_partition_size()
  btrfs-progs: add error handling for
    device_get_partition_size_fd_stat()

Zoltan Racz (1):
  btrfs-progs: fix the wrong size from device_get_partition_size_sysfs()

 check/main.c            |  8 ++++-
 check/mode-lowmem.c     |  9 ++++-
 cmds/filesystem-usage.c | 15 ++++++---
 cmds/replace.c          | 14 ++++++--
 common/device-utils.c   | 74 ++++++++++++++++++-----------------------
 common/device-utils.h   |  5 ++-
 kernel-shared/volumes.c | 11 ++++--
 kernel-shared/zoned.c   | 18 +++++++---
 mkfs/common.c           | 39 ++++------------------
 mkfs/common.h           |  1 -
 mkfs/main.c             | 14 +++++---
 11 files changed, 110 insertions(+), 98 deletions(-)

--
2.50.1

[PATCH v2 1/6] btrfs-progs: fix the wrong size from device_get_partition_size_sysfs()

[Qu Wenruo]

From: Zoltan Racz <racz.zoli@gmail.com>

[BUG]
When an unprivileged user, who can not access the block device, run
"btrfs dev usage", it's very common to result the following incorrect
output:

  $ btrfs dev usage /mnt/btrfs/
  WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
  /dev/mapper/test-scratch1, ID: 1
     Device size:            20.00MiB <<<
     Device slack:           16.00EiB <<<
     Unallocated:                 N/A

Note if the unprivileged user has read access to the raw block file, it
will work as expected:

  $ btrfs dev usage /mnt/btrfs/
  WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
  /dev/mapper/test-scratch1, ID: 1
     Device size:            10.00GiB
     Device slack:              0.00B
     Unallocated:                 N/A

[CAUSE]
When device_get_partition_size() is called, firstly the function checks
if we can do a read-only open() on the block device.

However under most distros, block devices are only accessible by root
and "disk" group.

If the unprivileged user is not in "disk" group, the open() will fail
and we have to fallback to device_get_partition_size_sysfs() as the
fallback.

The function device_get_partition_size_sysfs() will use
"/sys/block/<device>/size" as the size of the disk.

But according to the kernel source code, the "size" attribute is
implemented by returning bdev_nr_sectors(), and that result is always in
sector unit (512 bytes).

So if device_get_partition_size_sysfs() returns the value directly, it's
512 times smaller than the original size, causing errors.

[FIX]
Just do the proper left shift to return size in bytes.

Issue: #979
---
 common/device-utils.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/common/device-utils.c b/common/device-utils.c
index 783d79555446..bca392568d1b 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -375,7 +375,9 @@ static u64 device_get_partition_size_sysfs(const char *dev)
 		return 0;
 	}
 	close(sysfd);
-	return size;
+
+	/* <device>/size value is in sector (512B) unit. */
+	return size << SECTOR_SHIFT;
 }
 
 u64 device_get_partition_size(const char *dev)
-- 
2.50.1

[PATCH v2 2/6] btrfs-progs: remove the unnecessary device_get_partition_size() call

[Qu Wenruo]

Inside function _cmd_filesystem_usage_tabular(), there is a call of
device_get_partition_size() to calculate @unused.

However immediately after that call, @unused is updated using
devinfo->size, so that the previous call of device_get_partition_size()
is completely useless.

Just remove the unnecessary call.

And since we're here, also remove a dead newline in that loop at
variable declaration.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/filesystem-usage.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index 473479a1d72d..f812af13482e 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -959,7 +959,6 @@ static void _cmd_filesystem_usage_tabular(unsigned unit_mode,
 		int k;
 		const char *p;
 		const struct device_info *devinfo = devinfos->data[i];
-
 		u64  total_allocated = 0, unused;
 
 		p = strrchr(devinfo->path, '/');
@@ -1000,7 +999,6 @@ static void _cmd_filesystem_usage_tabular(unsigned unit_mode,
 			col++;
 		}
 
-		unused = device_get_partition_size(devinfo->path) - total_allocated;
 		unused = devinfo->size - total_allocated;
 
 		table_printf(matrix, unallocated_col, vhdr_skip + i, ">%s",
-- 
2.50.1

[PATCH v2 3/6] btrfs-progs: remove device_get_partition_size_fd()

[Qu Wenruo]

The last user of the helper is removed in commit 585ac14d1af2
("btrfs-progs: use btrfs_device_size() instead of
device_get_partition_size_fd()"), and that helper is not generic enough
to handle regular files.

So let's just remove it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 common/device-utils.c | 13 -------------
 common/device-utils.h |  1 -
 2 files changed, 14 deletions(-)

diff --git a/common/device-utils.c b/common/device-utils.c
index bca392568d1b..b32bd2cec1f1 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -329,19 +329,6 @@ u64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
 	return 0;
 }
 
-/*
- * Read partition size using the low-level ioctl
- */
-u64 device_get_partition_size_fd(int fd)
-{
-	u64 result;
-
-	if (ioctl(fd, BLKGETSIZE64, &result) < 0)
-		return 0;
-
-	return result;
-}
-
 static u64 device_get_partition_size_sysfs(const char *dev)
 {
 	int ret;
diff --git a/common/device-utils.h b/common/device-utils.h
index cef9405f3a9a..82e6ba547585 100644
--- a/common/device-utils.h
+++ b/common/device-utils.h
@@ -43,7 +43,6 @@ 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);
-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);
 u64 device_get_zone_unusable(int fd, u64 flags);
-- 
2.50.1

[PATCH v2 4/6] btrfs-progs: remove is_vol_small() helper

[Qu Wenruo]

The last user of the helper is removed in commit c11e36a29e84
("Btrfs-progs: Do not force mixed block group creation unless '-M'
option is specified").

So there is no one using this helper, and we can safely remove it.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 mkfs/common.c | 29 -----------------------------
 mkfs/common.h |  1 -
 2 files changed, 30 deletions(-)

diff --git a/mkfs/common.c b/mkfs/common.c
index d7a1dc5867eb..c5f73de81194 100644
--- a/mkfs/common.c
+++ b/mkfs/common.c
@@ -1167,35 +1167,6 @@ bool test_status_for_mkfs(const char *file, bool force_overwrite)
 	return false;
 }
 
-int is_vol_small(const char *file)
-{
-	int fd = -1;
-	int e;
-	struct stat st;
-	u64 size;
-
-	fd = open(file, O_RDONLY);
-	if (fd < 0)
-		return -errno;
-	if (fstat(fd, &st) < 0) {
-		e = -errno;
-		close(fd);
-		return e;
-	}
-	size = device_get_partition_size_fd_stat(fd, &st);
-	if (size == 0) {
-		close(fd);
-		return -1;
-	}
-	if (size < BTRFS_MKFS_SMALL_VOLUME_SIZE) {
-		close(fd);
-		return 1;
-	} else {
-		close(fd);
-		return 0;
-	}
-}
-
 int test_minimum_size(const char *file, u64 min_dev_size)
 {
 	int fd;
diff --git a/mkfs/common.h b/mkfs/common.h
index c600c16622fa..d08a5fd87203 100644
--- a/mkfs/common.h
+++ b/mkfs/common.h
@@ -106,7 +106,6 @@ int make_btrfs(int fd, struct btrfs_mkfs_config *cfg);
 u64 btrfs_min_dev_size(u32 nodesize, bool mixed, u64 zone_size, u64 meta_profile,
 		       u64 data_profile);
 int test_minimum_size(const char *file, u64 min_dev_size);
-int is_vol_small(const char *file);
 int test_num_disk_vs_raid(u64 metadata_profile, u64 data_profile,
 	u64 dev_cnt, int mixed, int ssd);
 bool test_status_for_mkfs(const char *file, bool force_overwrite);
-- 
2.50.1

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

[Qu Wenruo]

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 return s64, as even the kernel won't return a
block size larger than LLONG_MAX.
Thus we're safe to use the minus range of s64 to indicate an error.

The only exception is load_device_info() which will only give a warning
and continue.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 cmds/filesystem-usage.c | 13 +++++++++++--
 cmds/replace.c          | 14 ++++++++++++--
 common/device-utils.c   | 34 ++++++++++++++++------------------
 common/device-utils.h   |  2 +-
 4 files changed, 40 insertions(+), 23 deletions(-)

diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
index f812af13482e..755f540ac0bf 100644
--- a/cmds/filesystem-usage.c
+++ b/cmds/filesystem-usage.c
@@ -819,9 +819,18 @@ static int load_device_info(int fd, struct array *devinfos)
 		if (!dev_info.path[0]) {
 			strcpy(info->path, "missing");
 		} else {
+			s64 dev_size;
+
 			strcpy(info->path, (char *)dev_info.path);
-			info->device_size =
-				device_get_partition_size((const char *)dev_info.path);
+			dev_size = device_get_partition_size((const char *)dev_info.path);
+			if (dev_size < 0) {
+				errno = -dev_size;
+				warning("failed to get device size for %s: %m",
+					dev_info.path);
+				info->device_size = 0;
+			} else {
+				info->device_size = dev_size;
+			}
 		}
 		info->size = dev_info.total_bytes;
 		ndevs++;
diff --git a/cmds/replace.c b/cmds/replace.c
index 887c3251a725..bb8f81979389 100644
--- a/cmds/replace.c
+++ b/cmds/replace.c
@@ -136,8 +136,8 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 	bool force_using_targetdev = false;
 	u64 dstdev_block_count;
 	bool do_not_background = false;
-	u64 srcdev_size;
-	u64 dstdev_size;
+	s64 srcdev_size;
+	s64 dstdev_size;
 	bool enqueue = false;
 	bool discard = true;
 
@@ -270,6 +270,11 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 			     BTRFS_DEVICE_PATH_NAME_MAX + 1);
 		start_args.start.srcdevid = 0;
 		srcdev_size = device_get_partition_size(srcdev);
+		if (srcdev_size < 0) {
+			errno = -srcdev_size;
+			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;
@@ -280,6 +285,11 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
 		goto leave_with_error;
 
 	dstdev_size = device_get_partition_size(dstdev);
+	if (dstdev_size < 0) {
+		errno = -dstdev_size;
+		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 b32bd2cec1f1..8b545d171b18 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -329,7 +329,7 @@ u64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
 	return 0;
 }
 
-static u64 device_get_partition_size_sysfs(const char *dev)
+static s64 device_get_partition_size_sysfs(const char *dev)
 {
 	int ret;
 	char path[PATH_MAX] = {};
@@ -341,33 +341,30 @@ 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;
-	}
+	close(sysfd);
+	if (ret < 0)
+		return ret;
 	errno = 0;
 	size = strtoull(sizebuf, NULL, 10);
-	if (size == ULLONG_MAX && errno == ERANGE) {
-		close(sysfd);
-		return 0;
-	}
-	close(sysfd);
-
-	/* <device>/size value is in sector (512B) unit. */
+	if (size == ULLONG_MAX && errno == ERANGE)
+		return -ERANGE;
+	/* Extra overflow check. */
+	if (size > LLONG_MAX >> SECTOR_SHIFT)
+		return -ERANGE;
 	return size << SECTOR_SHIFT;
 }
 
-u64 device_get_partition_size(const char *dev)
+s64 device_get_partition_size(const char *dev)
 {
 	u64 result;
 	int fd = open(dev, O_RDONLY);
@@ -377,10 +374,11 @@ u64 device_get_partition_size(const char *dev)
 
 	if (ioctl(fd, BLKGETSIZE64, &result) < 0) {
 		close(fd);
-		return 0;
+		return -errno;
 	}
 	close(fd);
-
+	if (result > LLONG_MAX)
+		return -ERANGE;
 	return result;
 }
 
diff --git a/common/device-utils.h b/common/device-utils.h
index 82e6ba547585..2ada057adcd3 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);
+s64 device_get_partition_size(const char *dev);
 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);
 u64 device_get_zone_unusable(int fd, u64 flags);
-- 
2.50.1

[PATCH v2 6/6] btrfs-progs: add error handling for device_get_partition_size_fd_stat()

[Qu Wenruo]

The function device_get_partition_size_fd_state() has two different
error paths:

- The target file is not a regular nor block file
  This should be the common one.

- ioctl failed
  This should be very rare.

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 return s64, and since block device size should
be no larger than LLONG_MAX, it's safe to use the minus range to
indicate errors.

And since we're here, also enhance the error handling of the callers to
do an explicit error message.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 check/main.c            |  8 +++++++-
 check/mode-lowmem.c     |  9 ++++++++-
 common/device-utils.c   | 27 ++++++++++++++++-----------
 common/device-utils.h   |  2 +-
 kernel-shared/volumes.c | 11 ++++++++---
 kernel-shared/zoned.c   | 18 +++++++++++++-----
 mkfs/common.c           | 10 ++++++----
 mkfs/main.c             | 14 ++++++++++----
 8 files changed, 69 insertions(+), 30 deletions(-)

diff --git a/check/main.c b/check/main.c
index 84b6de597072..f0ca78bb2e19 100644
--- a/check/main.c
+++ b/check/main.c
@@ -5438,7 +5438,7 @@ static int process_device_item(struct rb_root *dev_cache,
 	device = btrfs_find_device_by_devid(gfs_info->fs_devices, rec->devid, 0);
 	if (device && device->fd >= 0) {
 		struct stat st;
-		u64 block_dev_size;
+		s64 block_dev_size;
 
 		ret = fstat(device->fd, &st);
 		if (ret < 0) {
@@ -5448,6 +5448,12 @@ static int process_device_item(struct rb_root *dev_cache,
 			goto skip;
 		}
 		block_dev_size = device_get_partition_size_fd_stat(device->fd, &st);
+		if (block_dev_size < 0) {
+			errno = -block_dev_size;
+			warning("failed to get device size for %s, skipping size check: %m",
+				device->name);
+			goto skip;
+		}
 		if (block_dev_size < rec->total_byte) {
 			error(
 "block device size is smaller than total_bytes in device item, has %llu expect >= %llu",
diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
index e05bb0da1709..8d1b4c3f3f2d 100644
--- a/check/mode-lowmem.c
+++ b/check/mode-lowmem.c
@@ -4716,7 +4716,7 @@ static int check_dev_item(struct extent_buffer *eb, int slot,
 	struct btrfs_dev_extent *ptr;
 	struct btrfs_device *dev;
 	struct stat st;
-	u64 block_dev_size;
+	s64 block_dev_size;
 	u64 total_bytes;
 	u64 dev_id;
 	u64 used;
@@ -4823,6 +4823,13 @@ next:
 		return 0;
 	}
 	block_dev_size = device_get_partition_size_fd_stat(dev->fd, &st);
+	if (block_dev_size < 0) {
+		errno = -block_dev_size;
+		warning(
+	"failed to get device size for %s, skipping its block device size check: %m",
+			dev->name);
+		return 0;
+	}
 	if (block_dev_size < total_bytes) {
 		error(
 "block device size is smaller than total_bytes in device item, has %llu expect >= %llu",
diff --git a/common/device-utils.c b/common/device-utils.c
index 8b545d171b18..b4daf7605fff 100644
--- a/common/device-utils.c
+++ b/common/device-utils.c
@@ -226,7 +226,7 @@ int btrfs_prepare_device(int fd, const char *file, u64 *byte_count_ret,
 			 u64 max_byte_count, unsigned opflags)
 {
 	struct btrfs_zoned_device_info *zinfo = NULL;
-	u64 byte_count;
+	s64 byte_count;
 	struct stat st;
 	int i, ret;
 
@@ -237,12 +237,13 @@ int btrfs_prepare_device(int fd, const char *file, u64 *byte_count_ret,
 	}
 
 	byte_count = device_get_partition_size_fd_stat(fd, &st);
-	if (byte_count == 0) {
-		error("unable to determine size of %s", file);
+	if (byte_count < 0) {
+		errno = -byte_count;
+		error("unable to determine size of %s: %m", file);
 		return 1;
 	}
 	if (max_byte_count)
-		byte_count = min(byte_count, max_byte_count);
+		byte_count = min_t(u64, byte_count, max_byte_count);
 
 	if (opflags & PREP_DEVICE_ZONED) {
 		ret = btrfs_get_zone_info(fd, file, &zinfo);
@@ -315,18 +316,22 @@ err:
 	return 1;
 }
 
-u64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
+s64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
 {
 	u64 size;
 
-	if (S_ISREG(st->st_mode))
+	if (S_ISREG(st->st_mode)) {
+		if (st->st_size > LLONG_MAX)
+			return -ERANGE;
 		return st->st_size;
+	}
 	if (!S_ISBLK(st->st_mode))
-		return 0;
-	if (ioctl(fd, BLKGETSIZE64, &size) >= 0)
-		return size;
-
-	return 0;
+		return -EINVAL;
+	if (ioctl(fd, BLKGETSIZE64, &size) < 0)
+		return -errno;
+	if (size > LLONG_MAX)
+		return -ERANGE;
+	return size;
 }
 
 static s64 device_get_partition_size_sysfs(const char *dev)
diff --git a/common/device-utils.h b/common/device-utils.h
index 2ada057adcd3..666dc3196e2f 100644
--- a/common/device-utils.h
+++ b/common/device-utils.h
@@ -43,7 +43,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);
 s64 device_get_partition_size(const char *dev);
-u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
+s64 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);
 u64 device_get_zone_unusable(int fd, u64 flags);
 u64 device_get_zone_size(int fd, const char *name);
diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
index be01bdb4d3f6..fccff07ba761 100644
--- a/kernel-shared/volumes.c
+++ b/kernel-shared/volumes.c
@@ -3081,7 +3081,7 @@ static int btrfs_fix_block_device_size(struct btrfs_fs_info *fs_info,
 				       struct btrfs_device *device)
 {
 	struct stat st;
-	u64 block_dev_size;
+	s64 block_dev_size;
 	int ret;
 
 	if (device->fd < 0 || !device->writeable) {
@@ -3096,8 +3096,13 @@ static int btrfs_fix_block_device_size(struct btrfs_fs_info *fs_info,
 		return -errno;
 	}
 
-	block_dev_size = round_down(device_get_partition_size_fd_stat(device->fd, &st),
-				    fs_info->sectorsize);
+	block_dev_size = device_get_partition_size_fd_stat(device->fd, &st);
+	if (block_dev_size < 0) {
+		errno = -block_dev_size;
+		error("failed to get device size for %s: %m", device->name);
+		return -errno;
+	}
+	block_dev_size = round_down(block_dev_size, fs_info->sectorsize);
 
 	/*
 	 * Total_bytes in device item is no larger than the device block size,
diff --git a/kernel-shared/zoned.c b/kernel-shared/zoned.c
index d96311af70b2..1036dbd153ad 100644
--- a/kernel-shared/zoned.c
+++ b/kernel-shared/zoned.c
@@ -166,7 +166,8 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
 {
 	const sector_t zone_sectors = emulated_zone_size >> SECTOR_SHIFT;
 	struct stat st;
-	sector_t bdev_size;
+	s64 bdev_size;
+	sector_t bdev_nr_sectors;
 	unsigned int i;
 	int ret;
 
@@ -176,7 +177,13 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
 		return -EIO;
 	}
 
-	bdev_size = device_get_partition_size_fd_stat(fd, &st) >> SECTOR_SHIFT;
+	bdev_size = device_get_partition_size_fd_stat(fd, &st);
+	if (bdev_size < 0) {
+		errno = -bdev_size;
+		error("failed to get device size for %s: %m", file);
+		return bdev_size;
+	}
+	bdev_nr_sectors = bdev_size >> SECTOR_SHIFT;
 
 	pos >>= SECTOR_SHIFT;
 	for (i = 0; i < nr_zones; i++) {
@@ -187,7 +194,7 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
 		zones[i].type = BLK_ZONE_TYPE_CONVENTIONAL;
 		zones[i].cond = BLK_ZONE_COND_NOT_WP;
 
-		if (zones[i].wp >= bdev_size) {
+		if (zones[i].wp >= bdev_nr_sectors) {
 			i++;
 			break;
 		}
@@ -289,7 +296,7 @@ int btrfs_reset_dev_zone(int fd, struct blk_zone *zone)
 static int report_zones(int fd, const char *file,
 			struct btrfs_zoned_device_info *zinfo)
 {
-	u64 device_size;
+	s64 device_size;
 	u64 zone_bytes = zone_size(file);
 	size_t rep_size;
 	u64 sector = 0;
@@ -326,7 +333,8 @@ static int report_zones(int fd, const char *file,
 	}
 
 	device_size = device_get_partition_size_fd_stat(fd, &st);
-	if (device_size == 0) {
+	if (device_size < 0) {
+		errno = -device_size;
 		error("zoned: failed to read size of %s: %m", file);
 		exit(1);
 	}
diff --git a/mkfs/common.c b/mkfs/common.c
index c5f73de81194..12a9a0bd8176 100644
--- a/mkfs/common.c
+++ b/mkfs/common.c
@@ -1171,6 +1171,7 @@ int test_minimum_size(const char *file, u64 min_dev_size)
 {
 	int fd;
 	struct stat statbuf;
+	s64 size;
 
 	fd = open(file, O_RDONLY);
 	if (fd < 0)
@@ -1179,11 +1180,12 @@ int test_minimum_size(const char *file, u64 min_dev_size)
 		close(fd);
 		return -errno;
 	}
-	if (device_get_partition_size_fd_stat(fd, &statbuf) < min_dev_size) {
-		close(fd);
-		return 1;
-	}
+	size = device_get_partition_size_fd_stat(fd, &statbuf);
 	close(fd);
+	if (size < 0)
+		return size;
+	if (size < min_dev_size)
+		return 1;
 	return 0;
 }
 
diff --git a/mkfs/main.c b/mkfs/main.c
index c3529044a836..166a4fc0fd32 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1254,7 +1254,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 	bool metadata_profile_set = false;
 	u64 data_profile = 0;
 	bool data_profile_set = false;
-	u64 byte_count = 0;
+	s64 byte_count = 0;
 	u64 dev_byte_count = 0;
 	bool mixed = false;
 	char *label = NULL;
@@ -1818,9 +1818,15 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
 		 * Block_count not specified, use file/device size first.
 		 * Or we will always use source_dir_size calculated for mkfs.
 		 */
-		if (!byte_count)
-			byte_count = round_down(device_get_partition_size_fd_stat(fd, &statbuf),
-						sectorsize);
+		if (!byte_count) {
+			byte_count = device_get_partition_size_fd_stat(fd, &statbuf);
+			if (byte_count < 0) {
+				errno = -byte_count;
+				error("failed to get device size for %s: %m", file);
+				goto error;
+			}
+			byte_count = round_down(byte_count, sectorsize);
+		}
 		source_dir_size = btrfs_mkfs_size_dir(source_dir, sectorsize,
 				min_dev_size, metadata_profile, data_profile);
 		UASSERT(IS_ALIGNED(source_dir_size, sectorsize));
-- 
2.50.1

Re: [PATCH v2 1/6] btrfs-progs: fix the wrong size from device_get_partition_size_sysfs()

[Anand Jain]

On 6/8/25 12:48, Qu Wenruo wrote:
> From: Zoltan Racz <racz.zoli@gmail.com>
> 
> [BUG]
> When an unprivileged user, who can not access the block device, run
> "btrfs dev usage", it's very common to result the following incorrect
> output:
> 
>    $ btrfs dev usage /mnt/btrfs/
>    WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
>    /dev/mapper/test-scratch1, ID: 1
>       Device size:            20.00MiB <<<
>       Device slack:           16.00EiB <<<
>       Unallocated:                 N/A
> 
> Note if the unprivileged user has read access to the raw block file, it
> will work as expected:
> 
>    $ btrfs dev usage /mnt/btrfs/
>    WARNING: cannot read detailed chunk info, per-device usage will not be shown, run as root
>    /dev/mapper/test-scratch1, ID: 1
>       Device size:            10.00GiB
>       Device slack:              0.00B
>       Unallocated:                 N/A
> 
> [CAUSE]
> When device_get_partition_size() is called, firstly the function checks
> if we can do a read-only open() on the block device.
> 
> However under most distros, block devices are only accessible by root
> and "disk" group.
> 
> If the unprivileged user is not in "disk" group, the open() will fail
> and we have to fallback to device_get_partition_size_sysfs() as the
> fallback.
> 
> The function device_get_partition_size_sysfs() will use
> "/sys/block/<device>/size" as the size of the disk.
> 
> But according to the kernel source code, the "size" attribute is
> implemented by returning bdev_nr_sectors(), and that result is always in
> sector unit (512 bytes).
> 
> So if device_get_partition_size_sysfs() returns the value directly, it's
> 512 times smaller than the original size, causing errors.
> 
> [FIX]
> Just do the proper left shift to return size in bytes.
> 
> Issue: #979
> ---

SOB is missing.

Changes looks good.

>   common/device-utils.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/common/device-utils.c b/common/device-utils.c
> index 783d79555446..bca392568d1b 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -375,7 +375,9 @@ static u64 device_get_partition_size_sysfs(const char *dev)
>   		return 0;
>   	}
>   	close(sysfd);
> -	return size;
> +
> +	/* <device>/size value is in sector (512B) unit. */
> +	return size << SECTOR_SHIFT;
>   }
>   
>   u64 device_get_partition_size(const char *dev)

Re: [PATCH v2 2/6] btrfs-progs: remove the unnecessary device_get_partition_size() call

[Anand Jain]

On 6/8/25 12:48, Qu Wenruo wrote:
> Inside function _cmd_filesystem_usage_tabular(), there is a call of
> device_get_partition_size() to calculate @unused.
> 
> However immediately after that call, @unused is updated using
> devinfo->size, so that the previous call of device_get_partition_size()
> is completely useless.
> 
> Just remove the unnecessary call.
> 
> And since we're here, also remove a dead newline in that loop at
> variable declaration.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   cmds/filesystem-usage.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> index 473479a1d72d..f812af13482e 100644
> --- a/cmds/filesystem-usage.c
> +++ b/cmds/filesystem-usage.c
> @@ -959,7 +959,6 @@ static void _cmd_filesystem_usage_tabular(unsigned unit_mode,
>   		int k;
>   		const char *p;
>   		const struct device_info *devinfo = devinfos->data[i];
> -
>   		u64  total_allocated = 0, unused;
>   
>   		p = strrchr(devinfo->path, '/');
> @@ -1000,7 +999,6 @@ static void _cmd_filesystem_usage_tabular(unsigned unit_mode,
>   			col++;
>   		}
>   
> -		unused = device_get_partition_size(devinfo->path) - total_allocated;
>   		unused = devinfo->size - total_allocated;
>   
>   		table_printf(matrix, unallocated_col, vhdr_skip + i, ">%s",


Fixes:

commit 7ab0c46a814a719fc88ddffbdc81b29d5c946fa2
     btrfs-progs: fi usage: fix unallocated and print total and slack


Reviewed-by: Anand Jain <anand.jain@oracle.com>

Re: [PATCH v2 3/6] btrfs-progs: remove device_get_partition_size_fd()

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>


Thanks.

Re: [PATCH v2 1/6] btrfs-progs: fix the wrong size from device_get_partition_size_sysfs()

[Qu Wenruo]

在 2025/8/8 15:17, Anand Jain 写道:
> On 6/8/25 12:48, Qu Wenruo wrote:
>> From: Zoltan Racz <racz.zoli@gmail.com>
>>
>> [BUG]
>> When an unprivileged user, who can not access the block device, run
>> "btrfs dev usage", it's very common to result the following incorrect
>> output:
>>
>>    $ btrfs dev usage /mnt/btrfs/
>>    WARNING: cannot read detailed chunk info, per-device usage will not 
>> be shown, run as root
>>    /dev/mapper/test-scratch1, ID: 1
>>       Device size:            20.00MiB <<<
>>       Device slack:           16.00EiB <<<
>>       Unallocated:                 N/A
>>
>> Note if the unprivileged user has read access to the raw block file, it
>> will work as expected:
>>
>>    $ btrfs dev usage /mnt/btrfs/
>>    WARNING: cannot read detailed chunk info, per-device usage will not 
>> be shown, run as root
>>    /dev/mapper/test-scratch1, ID: 1
>>       Device size:            10.00GiB
>>       Device slack:              0.00B
>>       Unallocated:                 N/A
>>
>> [CAUSE]
>> When device_get_partition_size() is called, firstly the function checks
>> if we can do a read-only open() on the block device.
>>
>> However under most distros, block devices are only accessible by root
>> and "disk" group.
>>
>> If the unprivileged user is not in "disk" group, the open() will fail
>> and we have to fallback to device_get_partition_size_sysfs() as the
>> fallback.
>>
>> The function device_get_partition_size_sysfs() will use
>> "/sys/block/<device>/size" as the size of the disk.
>>
>> But according to the kernel source code, the "size" attribute is
>> implemented by returning bdev_nr_sectors(), and that result is always in
>> sector unit (512 bytes).
>>
>> So if device_get_partition_size_sysfs() returns the value directly, it's
>> 512 times smaller than the original size, causing errors.
>>
>> [FIX]
>> Just do the proper left shift to return size in bytes.
>>
>> Issue: #979
>> ---
> 
> SOB is missing.

For progs, SOB really depends on the author.
It's recommended for long time contributors, but for new contributors 
it's fine to skip the SOB.

Unless you mean my SOB, but in this particular case since the author 
didn't provide SOB, I didn't want to have only my SOB.
This will give an impression that I'm the only one contributing to the 
patch.

Thus I didn't leave my SOB either.

Thanks,
Qu
> 
> Changes looks good.
> 
>>   common/device-utils.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/common/device-utils.c b/common/device-utils.c
>> index 783d79555446..bca392568d1b 100644
>> --- a/common/device-utils.c
>> +++ b/common/device-utils.c
>> @@ -375,7 +375,9 @@ static u64 device_get_partition_size_sysfs(const 
>> char *dev)
>>           return 0;
>>       }
>>       close(sysfd);
>> -    return size;
>> +
>> +    /* <device>/size value is in sector (512B) unit. */
>> +    return size << SECTOR_SHIFT;
>>   }
>>   u64 device_get_partition_size(const char *dev)
> 

Re: [PATCH v2 4/6] btrfs-progs: remove is_vol_small() helper

[Anand Jain]

Reviewed-by: Anand Jain <anand.jain@oracle.com>

Thanks

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

[Anand Jain]

On 6/8/25 12:48, 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 return s64, as even the kernel won't return a
> block size larger than LLONG_MAX.
> Thus we're safe to use the minus range of s64 to indicate an error.

Returning s64 is almost unused in btrfs-progs; Either PTR_ERR() or
int return + arg parameter * u64; Rest looks good.

Thanks, Anand

> The only exception is load_device_info() which will only give a warning
> and continue.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   cmds/filesystem-usage.c | 13 +++++++++++--
>   cmds/replace.c          | 14 ++++++++++++--
>   common/device-utils.c   | 34 ++++++++++++++++------------------
>   common/device-utils.h   |  2 +-
>   4 files changed, 40 insertions(+), 23 deletions(-)
> 
> diff --git a/cmds/filesystem-usage.c b/cmds/filesystem-usage.c
> index f812af13482e..755f540ac0bf 100644
> --- a/cmds/filesystem-usage.c
> +++ b/cmds/filesystem-usage.c
> @@ -819,9 +819,18 @@ static int load_device_info(int fd, struct array *devinfos)
>   		if (!dev_info.path[0]) {
>   			strcpy(info->path, "missing");
>   		} else {
> +			s64 dev_size;
> +
>   			strcpy(info->path, (char *)dev_info.path);
> -			info->device_size =
> -				device_get_partition_size((const char *)dev_info.path);
> +			dev_size = device_get_partition_size((const char *)dev_info.path);
> +			if (dev_size < 0) {
> +				errno = -dev_size;
> +				warning("failed to get device size for %s: %m",
> +					dev_info.path);
> +				info->device_size = 0;
> +			} else {
> +				info->device_size = dev_size;
> +			}
>   		}
>   		info->size = dev_info.total_bytes;
>   		ndevs++;
> diff --git a/cmds/replace.c b/cmds/replace.c
> index 887c3251a725..bb8f81979389 100644
> --- a/cmds/replace.c
> +++ b/cmds/replace.c
> @@ -136,8 +136,8 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>   	bool force_using_targetdev = false;
>   	u64 dstdev_block_count;
>   	bool do_not_background = false;
> -	u64 srcdev_size;
> -	u64 dstdev_size;
> +	s64 srcdev_size;
> +	s64 dstdev_size;
>   	bool enqueue = false;
>   	bool discard = true;
>   
> @@ -270,6 +270,11 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>   			     BTRFS_DEVICE_PATH_NAME_MAX + 1);
>   		start_args.start.srcdevid = 0;
>   		srcdev_size = device_get_partition_size(srcdev);
> +		if (srcdev_size < 0) {
> +			errno = -srcdev_size;
> +			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;
> @@ -280,6 +285,11 @@ static int cmd_replace_start(const struct cmd_struct *cmd,
>   		goto leave_with_error;
>   
>   	dstdev_size = device_get_partition_size(dstdev);
> +	if (dstdev_size < 0) {
> +		errno = -dstdev_size;
> +		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 b32bd2cec1f1..8b545d171b18 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -329,7 +329,7 @@ u64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
>   	return 0;
>   }
>   
> -static u64 device_get_partition_size_sysfs(const char *dev)
> +static s64 device_get_partition_size_sysfs(const char *dev)
>   {
>   	int ret;
>   	char path[PATH_MAX] = {};
> @@ -341,33 +341,30 @@ 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;
> -	}
> +	close(sysfd);
> +	if (ret < 0)
> +		return ret;
>   	errno = 0;
>   	size = strtoull(sizebuf, NULL, 10);
> -	if (size == ULLONG_MAX && errno == ERANGE) {
> -		close(sysfd);
> -		return 0;
> -	}
> -	close(sysfd);
> -
> -	/* <device>/size value is in sector (512B) unit. */
> +	if (size == ULLONG_MAX && errno == ERANGE)
> +		return -ERANGE;
> +	/* Extra overflow check. */
> +	if (size > LLONG_MAX >> SECTOR_SHIFT)
> +		return -ERANGE;
>   	return size << SECTOR_SHIFT;
>   }
>   
> -u64 device_get_partition_size(const char *dev)
> +s64 device_get_partition_size(const char *dev)
>   {
>   	u64 result;
>   	int fd = open(dev, O_RDONLY);
> @@ -377,10 +374,11 @@ u64 device_get_partition_size(const char *dev)
>   
>   	if (ioctl(fd, BLKGETSIZE64, &result) < 0) {
>   		close(fd);
> -		return 0;
> +		return -errno;
>   	}
>   	close(fd);
> -
> +	if (result > LLONG_MAX)
> +		return -ERANGE;
>   	return result;
>   }
>   
> diff --git a/common/device-utils.h b/common/device-utils.h
> index 82e6ba547585..2ada057adcd3 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);
> +s64 device_get_partition_size(const char *dev);
>   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);
>   u64 device_get_zone_unusable(int fd, u64 flags);

Re: [PATCH v2 6/6] btrfs-progs: add error handling for device_get_partition_size_fd_stat()

[Anand Jain]

On 6/8/25 12:48, Qu Wenruo wrote:
> The function device_get_partition_size_fd_state() has two different
> error paths:
> 
> - The target file is not a regular nor block file
>    This should be the common one.
> 
> - ioctl failed
>    This should be very rare.
> 
> 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 return s64, and since block device size should
> be no larger than LLONG_MAX, it's safe to use the minus range to
> indicate errors.

As commented in the patch 5/6 also, signed64 as return is avoided in 
btrfs-progs.

Thanks, Anand


> And since we're here, also enhance the error handling of the callers to
> do an explicit error message.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>   check/main.c            |  8 +++++++-
>   check/mode-lowmem.c     |  9 ++++++++-
>   common/device-utils.c   | 27 ++++++++++++++++-----------
>   common/device-utils.h   |  2 +-
>   kernel-shared/volumes.c | 11 ++++++++---
>   kernel-shared/zoned.c   | 18 +++++++++++++-----
>   mkfs/common.c           | 10 ++++++----
>   mkfs/main.c             | 14 ++++++++++----
>   8 files changed, 69 insertions(+), 30 deletions(-)
> 
> diff --git a/check/main.c b/check/main.c
> index 84b6de597072..f0ca78bb2e19 100644
> --- a/check/main.c
> +++ b/check/main.c
> @@ -5438,7 +5438,7 @@ static int process_device_item(struct rb_root *dev_cache,
>   	device = btrfs_find_device_by_devid(gfs_info->fs_devices, rec->devid, 0);
>   	if (device && device->fd >= 0) {
>   		struct stat st;
> -		u64 block_dev_size;
> +		s64 block_dev_size;
>   
>   		ret = fstat(device->fd, &st);
>   		if (ret < 0) {
> @@ -5448,6 +5448,12 @@ static int process_device_item(struct rb_root *dev_cache,
>   			goto skip;
>   		}
>   		block_dev_size = device_get_partition_size_fd_stat(device->fd, &st);
> +		if (block_dev_size < 0) {
> +			errno = -block_dev_size;
> +			warning("failed to get device size for %s, skipping size check: %m",
> +				device->name);
> +			goto skip;
> +		}
>   		if (block_dev_size < rec->total_byte) {
>   			error(
>   "block device size is smaller than total_bytes in device item, has %llu expect >= %llu",
> diff --git a/check/mode-lowmem.c b/check/mode-lowmem.c
> index e05bb0da1709..8d1b4c3f3f2d 100644
> --- a/check/mode-lowmem.c
> +++ b/check/mode-lowmem.c
> @@ -4716,7 +4716,7 @@ static int check_dev_item(struct extent_buffer *eb, int slot,
>   	struct btrfs_dev_extent *ptr;
>   	struct btrfs_device *dev;
>   	struct stat st;
> -	u64 block_dev_size;
> +	s64 block_dev_size;
>   	u64 total_bytes;
>   	u64 dev_id;
>   	u64 used;
> @@ -4823,6 +4823,13 @@ next:
>   		return 0;
>   	}
>   	block_dev_size = device_get_partition_size_fd_stat(dev->fd, &st);
> +	if (block_dev_size < 0) {
> +		errno = -block_dev_size;
> +		warning(
> +	"failed to get device size for %s, skipping its block device size check: %m",
> +			dev->name);
> +		return 0;
> +	}
>   	if (block_dev_size < total_bytes) {
>   		error(
>   "block device size is smaller than total_bytes in device item, has %llu expect >= %llu",
> diff --git a/common/device-utils.c b/common/device-utils.c
> index 8b545d171b18..b4daf7605fff 100644
> --- a/common/device-utils.c
> +++ b/common/device-utils.c
> @@ -226,7 +226,7 @@ int btrfs_prepare_device(int fd, const char *file, u64 *byte_count_ret,
>   			 u64 max_byte_count, unsigned opflags)
>   {
>   	struct btrfs_zoned_device_info *zinfo = NULL;
> -	u64 byte_count;
> +	s64 byte_count;
>   	struct stat st;
>   	int i, ret;
>   
> @@ -237,12 +237,13 @@ int btrfs_prepare_device(int fd, const char *file, u64 *byte_count_ret,
>   	}
>   
>   	byte_count = device_get_partition_size_fd_stat(fd, &st);
> -	if (byte_count == 0) {
> -		error("unable to determine size of %s", file);
> +	if (byte_count < 0) {
> +		errno = -byte_count;
> +		error("unable to determine size of %s: %m", file);
>   		return 1;
>   	}
>   	if (max_byte_count)
> -		byte_count = min(byte_count, max_byte_count);
> +		byte_count = min_t(u64, byte_count, max_byte_count);
>   
>   	if (opflags & PREP_DEVICE_ZONED) {
>   		ret = btrfs_get_zone_info(fd, file, &zinfo);
> @@ -315,18 +316,22 @@ err:
>   	return 1;
>   }
>   
> -u64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
> +s64 device_get_partition_size_fd_stat(int fd, const struct stat *st)
>   {
>   	u64 size;
>   
> -	if (S_ISREG(st->st_mode))
> +	if (S_ISREG(st->st_mode)) {
> +		if (st->st_size > LLONG_MAX)
> +			return -ERANGE;
>   		return st->st_size;
> +	}
>   	if (!S_ISBLK(st->st_mode))
> -		return 0;
> -	if (ioctl(fd, BLKGETSIZE64, &size) >= 0)
> -		return size;
> -
> -	return 0;
> +		return -EINVAL;
> +	if (ioctl(fd, BLKGETSIZE64, &size) < 0)
> +		return -errno;
> +	if (size > LLONG_MAX)
> +		return -ERANGE;
> +	return size;
>   }
>   
>   static s64 device_get_partition_size_sysfs(const char *dev)
> diff --git a/common/device-utils.h b/common/device-utils.h
> index 2ada057adcd3..666dc3196e2f 100644
> --- a/common/device-utils.h
> +++ b/common/device-utils.h
> @@ -43,7 +43,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);
>   s64 device_get_partition_size(const char *dev);
> -u64 device_get_partition_size_fd_stat(int fd, const struct stat *st);
> +s64 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);
>   u64 device_get_zone_unusable(int fd, u64 flags);
>   u64 device_get_zone_size(int fd, const char *name);
> diff --git a/kernel-shared/volumes.c b/kernel-shared/volumes.c
> index be01bdb4d3f6..fccff07ba761 100644
> --- a/kernel-shared/volumes.c
> +++ b/kernel-shared/volumes.c
> @@ -3081,7 +3081,7 @@ static int btrfs_fix_block_device_size(struct btrfs_fs_info *fs_info,
>   				       struct btrfs_device *device)
>   {
>   	struct stat st;
> -	u64 block_dev_size;
> +	s64 block_dev_size;
>   	int ret;
>   
>   	if (device->fd < 0 || !device->writeable) {
> @@ -3096,8 +3096,13 @@ static int btrfs_fix_block_device_size(struct btrfs_fs_info *fs_info,
>   		return -errno;
>   	}
>   
> -	block_dev_size = round_down(device_get_partition_size_fd_stat(device->fd, &st),
> -				    fs_info->sectorsize);
> +	block_dev_size = device_get_partition_size_fd_stat(device->fd, &st);
> +	if (block_dev_size < 0) {
> +		errno = -block_dev_size;
> +		error("failed to get device size for %s: %m", device->name);
> +		return -errno;
> +	}
> +	block_dev_size = round_down(block_dev_size, fs_info->sectorsize);
>   
>   	/*
>   	 * Total_bytes in device item is no larger than the device block size,
> diff --git a/kernel-shared/zoned.c b/kernel-shared/zoned.c
> index d96311af70b2..1036dbd153ad 100644
> --- a/kernel-shared/zoned.c
> +++ b/kernel-shared/zoned.c
> @@ -166,7 +166,8 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
>   {
>   	const sector_t zone_sectors = emulated_zone_size >> SECTOR_SHIFT;
>   	struct stat st;
> -	sector_t bdev_size;
> +	s64 bdev_size;
> +	sector_t bdev_nr_sectors;
>   	unsigned int i;
>   	int ret;
>   
> @@ -176,7 +177,13 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
>   		return -EIO;
>   	}
>   
> -	bdev_size = device_get_partition_size_fd_stat(fd, &st) >> SECTOR_SHIFT;
> +	bdev_size = device_get_partition_size_fd_stat(fd, &st);
> +	if (bdev_size < 0) {
> +		errno = -bdev_size;
> +		error("failed to get device size for %s: %m", file);
> +		return bdev_size;
> +	}
> +	bdev_nr_sectors = bdev_size >> SECTOR_SHIFT;
>   
>   	pos >>= SECTOR_SHIFT;
>   	for (i = 0; i < nr_zones; i++) {
> @@ -187,7 +194,7 @@ static int emulate_report_zones(const char *file, int fd, u64 pos,
>   		zones[i].type = BLK_ZONE_TYPE_CONVENTIONAL;
>   		zones[i].cond = BLK_ZONE_COND_NOT_WP;
>   
> -		if (zones[i].wp >= bdev_size) {
> +		if (zones[i].wp >= bdev_nr_sectors) {
>   			i++;
>   			break;
>   		}
> @@ -289,7 +296,7 @@ int btrfs_reset_dev_zone(int fd, struct blk_zone *zone)
>   static int report_zones(int fd, const char *file,
>   			struct btrfs_zoned_device_info *zinfo)
>   {
> -	u64 device_size;
> +	s64 device_size;
>   	u64 zone_bytes = zone_size(file);
>   	size_t rep_size;
>   	u64 sector = 0;
> @@ -326,7 +333,8 @@ static int report_zones(int fd, const char *file,
>   	}
>   
>   	device_size = device_get_partition_size_fd_stat(fd, &st);
> -	if (device_size == 0) {
> +	if (device_size < 0) {
> +		errno = -device_size;
>   		error("zoned: failed to read size of %s: %m", file);
>   		exit(1);
>   	}
> diff --git a/mkfs/common.c b/mkfs/common.c
> index c5f73de81194..12a9a0bd8176 100644
> --- a/mkfs/common.c
> +++ b/mkfs/common.c
> @@ -1171,6 +1171,7 @@ int test_minimum_size(const char *file, u64 min_dev_size)
>   {
>   	int fd;
>   	struct stat statbuf;
> +	s64 size;
>   
>   	fd = open(file, O_RDONLY);
>   	if (fd < 0)
> @@ -1179,11 +1180,12 @@ int test_minimum_size(const char *file, u64 min_dev_size)
>   		close(fd);
>   		return -errno;
>   	}
> -	if (device_get_partition_size_fd_stat(fd, &statbuf) < min_dev_size) {
> -		close(fd);
> -		return 1;
> -	}
> +	size = device_get_partition_size_fd_stat(fd, &statbuf);
>   	close(fd);
> +	if (size < 0)
> +		return size;
> +	if (size < min_dev_size)
> +		return 1;
>   	return 0;
>   }
>   
> diff --git a/mkfs/main.c b/mkfs/main.c
> index c3529044a836..166a4fc0fd32 100644
> --- a/mkfs/main.c
> +++ b/mkfs/main.c
> @@ -1254,7 +1254,7 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   	bool metadata_profile_set = false;
>   	u64 data_profile = 0;
>   	bool data_profile_set = false;
> -	u64 byte_count = 0;
> +	s64 byte_count = 0;
>   	u64 dev_byte_count = 0;
>   	bool mixed = false;
>   	char *label = NULL;
> @@ -1818,9 +1818,15 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   		 * Block_count not specified, use file/device size first.
>   		 * Or we will always use source_dir_size calculated for mkfs.
>   		 */
> -		if (!byte_count)
> -			byte_count = round_down(device_get_partition_size_fd_stat(fd, &statbuf),
> -						sectorsize);
> +		if (!byte_count) {
> +			byte_count = device_get_partition_size_fd_stat(fd, &statbuf);
> +			if (byte_count < 0) {
> +				errno = -byte_count;
> +				error("failed to get device size for %s: %m", file);
> +				goto error;
> +			}
> +			byte_count = round_down(byte_count, sectorsize);
> +		}
>   		source_dir_size = btrfs_mkfs_size_dir(source_dir, sectorsize,
>   				min_dev_size, metadata_profile, data_profile);
>   		UASSERT(IS_ALIGNED(source_dir_size, sectorsize));

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

[Anand Jain]

On 8/8/25 15:23, Anand Jain wrote:
> On 6/8/25 12:48, 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 return s64, as even the kernel won't return a
>> block size larger than LLONG_MAX.
>> Thus we're safe to use the minus range of s64 to indicate an error.
> 

> Returning s64 is almost unused in btrfs-progs; Either PTR_ERR() or
> int return + arg parameter * u64; Rest looks good.

correction: almost unused -> mostly avoided

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

[Qu Wenruo]

在 2025/8/8 18:56, Anand Jain 写道:
> On 8/8/25 15:23, Anand Jain wrote:
>> On 6/8/25 12:48, 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 return s64, as even the kernel won't return a
>>> block size larger than LLONG_MAX.
>>> Thus we're safe to use the minus range of s64 to indicate an error.
>>
> 
>> Returning s64 is almost unused in btrfs-progs; Either PTR_ERR() or
>> int return + arg parameter * u64; Rest looks good.
> 
> correction: almost unused -> mostly avoided

@Boris, mind me to revert back to the old int + u64 *ret solution?

Despite the fact that s64 is seldomly used in progs, I also feel a 
little uneasy with the s64->u64 (all the extra ASSERT() I added) or the 
s64->int error code conversion.

Thanks,
Qu

Re: [PATCH v2 1/6] btrfs-progs: fix the wrong size from device_get_partition_size_sysfs()

[David Sterba]

On Fri, Aug 08, 2025 at 03:35:53PM +0930, Qu Wenruo wrote:
> 在 2025/8/8 15:17, Anand Jain 写道:
> > On 6/8/25 12:48, Qu Wenruo wrote:
> >> From: Zoltan Racz <racz.zoli@gmail.com>
> >>
> >> [BUG]
> >> When an unprivileged user, who can not access the block device, run
> >> "btrfs dev usage", it's very common to result the following incorrect
> >> output:
> >>
> >>    $ btrfs dev usage /mnt/btrfs/
> >>    WARNING: cannot read detailed chunk info, per-device usage will not 
> >> be shown, run as root
> >>    /dev/mapper/test-scratch1, ID: 1
> >>       Device size:            20.00MiB <<<
> >>       Device slack:           16.00EiB <<<
> >>       Unallocated:                 N/A
> >>
> >> Note if the unprivileged user has read access to the raw block file, it
> >> will work as expected:
> >>
> >>    $ btrfs dev usage /mnt/btrfs/
> >>    WARNING: cannot read detailed chunk info, per-device usage will not 
> >> be shown, run as root
> >>    /dev/mapper/test-scratch1, ID: 1
> >>       Device size:            10.00GiB
> >>       Device slack:              0.00B
> >>       Unallocated:                 N/A
> >>
> >> [CAUSE]
> >> When device_get_partition_size() is called, firstly the function checks
> >> if we can do a read-only open() on the block device.
> >>
> >> However under most distros, block devices are only accessible by root
> >> and "disk" group.
> >>
> >> If the unprivileged user is not in "disk" group, the open() will fail
> >> and we have to fallback to device_get_partition_size_sysfs() as the
> >> fallback.
> >>
> >> The function device_get_partition_size_sysfs() will use
> >> "/sys/block/<device>/size" as the size of the disk.
> >>
> >> But according to the kernel source code, the "size" attribute is
> >> implemented by returning bdev_nr_sectors(), and that result is always in
> >> sector unit (512 bytes).
> >>
> >> So if device_get_partition_size_sysfs() returns the value directly, it's
> >> 512 times smaller than the original size, causing errors.
> >>
> >> [FIX]
> >> Just do the proper left shift to return size in bytes.
> >>
> >> Issue: #979
> >> ---
> > 
> > SOB is missing.
> 
> For progs, SOB really depends on the author.
> It's recommended for long time contributors, but for new contributors 
> it's fine to skip the SOB.
> 
> Unless you mean my SOB, but in this particular case since the author 
> didn't provide SOB, I didn't want to have only my SOB.
> This will give an impression that I'm the only one contributing to the 
> patch.
> 
> Thus I didn't leave my SOB either.

It's a bit of a gray area. For kernel the SOB line has some meaning, in
progs it's been inherited but not manadatory.

Please still add your s-o-b if you're handling patch on somebody else's
behalf. This means that you have verified that the contribution is ok
from the project perspective. I used to add Author: tag, same as the
commit author but it seems redundant. If there's an issue or pull
request it's good to add it as well and that's what we can do for the
attribution.

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

[David Sterba]

On Fri, Aug 08, 2025 at 07:01:47PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/8/8 18:56, Anand Jain 写道:
> > On 8/8/25 15:23, Anand Jain wrote:
> >> On 6/8/25 12:48, 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 return s64, as even the kernel won't return a
> >>> block size larger than LLONG_MAX.
> >>> Thus we're safe to use the minus range of s64 to indicate an error.
> >>
> > 
> >> Returning s64 is almost unused in btrfs-progs; Either PTR_ERR() or
> >> int return + arg parameter * u64; Rest looks good.
> > 
> > correction: almost unused -> mostly avoided
> 
> @Boris, mind me to revert back to the old int + u64 *ret solution?

Please convert it to int return and *u64 for the parameter.

> Despite the fact that s64 is seldomly used in progs, I also feel a 
> little uneasy with the s64->u64 (all the extra ASSERT() I added) or the 
> s64->int error code conversion.

Yeah, size related things should stick to u64, differences use s64.

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

[Boris Burkov]

On Fri, Aug 08, 2025 at 07:01:47PM +0930, Qu Wenruo wrote:
> 
> 
> 在 2025/8/8 18:56, Anand Jain 写道:
> > On 8/8/25 15:23, Anand Jain wrote:
> > > On 6/8/25 12:48, 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 return s64, as even the kernel won't return a
> > > > block size larger than LLONG_MAX.
> > > > Thus we're safe to use the minus range of s64 to indicate an error.
> > > 
> > 
> > > Returning s64 is almost unused in btrfs-progs; Either PTR_ERR() or
> > > int return + arg parameter * u64; Rest looks good.
> > 
> > correction: almost unused -> mostly avoided
> 
> @Boris, mind me to revert back to the old int + u64 *ret solution?
> 

Yes, that's fine, sorry to send you on a yak shaving quest.

> Despite the fact that s64 is seldomly used in progs, I also feel a little
> uneasy with the s64->u64 (all the extra ASSERT() I added) or the s64->int
> error code conversion.
> 
> Thanks,
> Qu

Re: [PATCH v2 0/6] btrfs-progs: device_get_partition_size*() enhancement and cleanup

[David Sterba]

On Wed, Aug 06, 2025 at 02:18:49PM +0930, Qu Wenruo wrote:
> [CHANGELOG]
> v2:
> - Include and update Zoltan's fix to device_get_partition_size_sysfs()
>   The "/dev/block/<device>/size" attribute is always in sector unit
>   (512Bytes), independent from the physical device's block size.
> 
>   So the fix is pretty straightforward, just do the left shift before
>   returning.
> 
>   And update the commit message to properly reflect that.
> 
> - Use s64 as the return value of device_get_partition_size*()
>   Kernel ensures the max sector number of a device is LLONG_MAX >> 9,
>   so s64 is more than enough to cover the device size, and we are safe
>   to use the minus part to pass errors.
> 
> - Extra error handling when device_get_partition_size*() failed
>   Ouput an error message and exit.
>   Most callers are already doing that correctly but some are not.
> 
> - Keep the sysfs method as the fallback way to grab device size
>   Since the device_get_partition_size_sysfs() can be easily fixed, no
>   need to use complex path search for sector size.
> 
> - Add a new cleanup to remove is_vol_small()
> 
> Zoltan recently sent a fix to solve the bug in
> device_get_partition_size_sysfs(), which is causing problems for "btrfs
> dev usage".
> 
> It turns out that, the bug is just the attribute
> "/sys/block/<device>/size" is in sector unit (512B), not in bytes.
> 
> So fix the bug first by reporting the correct size in bytes.
> 
> Then remove several unused functions exposed during the error handling
> cleanup.
> 
> Finally cleanup device_get_partition_size*() helpers to provide a proper
> error handling, not just return 0 for errors, but proper minus erro
> code, and extra overflow check.
> 
> Qu Wenruo (5):
>   btrfs-progs: remove the unnecessary device_get_partition_size() call
>   btrfs-progs: remove device_get_partition_size_fd()
>   btrfs-progs: remove is_vol_small() helper
>   btrfs-progs: add error handling for device_get_partition_size()
>   btrfs-progs: add error handling for
>     device_get_partition_size_fd_stat()
> 
> Zoltan Racz (1):
>   btrfs-progs: fix the wrong size from device_get_partition_size_sysfs()

Besides the s64 conversion, the patches look good to me. Please add them
to devel, thanks.