Skip to contentSkip to conversation

mkfs.btrfs: round all device sizes to sectorsize

PATCH In review Eric Sandeen 3 participants 5 messages Jul 25, 2014 Btrfs file system

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 mkfs.btrfs: round all device sizes to sectorsize

make_btrfs() rounds down the first device size to a multiple of sectorsize:

	num_bytes = (num_bytes / sectorsize) * sectorsize;

but subsequent device adds don't.

This seems a bit odd & inconsistent, and it makes xfstest btrfs/011 _notrun(), because it explicitly checks that devices are the same size.

I don't know that there is anything inherently wrong with having a few device bytes extend past the last block, but to be consistent, it seems like btrfs_add_to_fsid() should round the size in the same way.

And now btrfs/011 runs more consistently; the test devices don't have to be sectorsize multiples in order for all mkfs'd device sizes to match.

---

ideally this might go into btrfs_device_size(), but we don't have the chosen sector size anywhere near there...

utils.c +1 -1
index e130849..4d7ee35 100644
@@ -554,7 +554,7 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
 	device->sector_size = sectorsize;
 	device->fd = fd;
 	device->writeable = 1;
-	device->total_bytes = block_count;
+	device->total_bytes = (block_count / sectorsize) * sectorsize;
 	device->bytes_used = 0;
 	device->total_ios = 0;
 	device->dev_root = root->fs_info->dev_root;
#1 lore raw
Quoted 29 lines · Eric Sandeen

make_btrfs() rounds down the first device size to a multiple of sectorsize:

	num_bytes = (num_bytes / sectorsize) * sectorsize;

but subsequent device adds don't.

This seems a bit odd & inconsistent, and it makes xfstest btrfs/011 _notrun(), because it explicitly checks that devices are the same size.

I don't know that there is anything inherently wrong with having a few device bytes extend past the last block, but to be consistent, it seems like btrfs_add_to_fsid() should round the size in the same way.

And now btrfs/011 runs more consistently; the test devices don't have to be sectorsize multiples in order for all mkfs'd device sizes to match.

---

ideally this might go into btrfs_device_size(), but we don't have the chosen sector size anywhere near there...

utils.c +0 -0
index e130849..4d7ee35 100644

utils.c int btrfs_add_to_fsid(struct btrfs_trans_handle *trans, · line 560

Quoted 8 lines
utils.c +1 -1
@@ -554,7 +554,7 @@ int btrfs_add_to_fsid(struct btrfs_trans_handle *trans,
  	device->sector_size = sectorsize;
  	device->fd = fd;
  	device->writeable = 1;
-	device->total_bytes = block_count;
+	device->total_bytes = (block_count / sectorsize) * sectorsize;
  	device->bytes_used = 0;
  	device->total_ios = 0;
  	device->dev_root = root->fs_info->dev_root;

its better to do this at the function btrfs_prepare_device() itself, so that it would include first and the rest added.
or
we need to apply this on the block_count instead, as we do add that to the total_bytes.

Thanks, Anand

Quoted 5 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
In reply to Eric Sandeen
Quoted 1 line · Eric Sandeen

make_btrfs() rounds down the first device size to a multiple of sectorsize:

               ^^^^^^^^^^^

utils.c int btrfs_add_to_fsid(struct btrfs_trans_handle *trans, · line 557

Quoted 2 lines
utils.c +1 -1
-	device->total_bytes = block_count;
+	device->total_bytes = (block_count / sectorsize) * sectorsize;

kerncompat.h:#define round_down(x, y) ((x) & ~__round_mask(x, y))

- z

#3 lore raw
Quoted 12 lines · Zach Brown
Quoted 1 line · Eric Sandeen

make_btrfs() rounds down the first device size to a multiple of sectorsize:

               ^^^^^^^^^^^

utils.c int btrfs_add_to_fsid(struct btrfs_trans_handle *trans, · line 557

Quoted 2 lines
utils.c +1 -1
-	device->total_bytes = block_count;
+	device->total_bytes = (block_count / sectorsize) * sectorsize;

kerncompat.h:#define round_down(x, y) ((x) & ~__round_mask(x, y))

- z

yeah yeah ;) this isn't copied kernel code but sure, that'd be better.

I'm trying to clean up this whole "we say blocks when we mean bytes!" thing, and I'll include round_down() as well.

Thanks,
-Eric

#4 lore raw
Quoted 11 lines · Eric Sandeen
Quoted 10 lines · Zach Brown
Quoted 1 line · Eric Sandeen

make_btrfs() rounds down the first device size to a multiple of sectorsize:

               ^^^^^^^^^^^

utils.c int btrfs_add_to_fsid(struct btrfs_trans_handle *trans, · line 557

Quoted 2 lines
utils.c +1 -1
-	device->total_bytes = block_count;
+	device->total_bytes = (block_count / sectorsize) * sectorsize;

kerncompat.h:#define round_down(x, y) ((x) & ~__round_mask(x, y))

Quoted 7 lines
Quoted 2 lines

- z

yeah yeah ;) this isn't copied kernel code but sure, that'd be better.

I'm trying to clean up this whole "we say blocks when we mean bytes!" thing, and I'll include round_down() as well.

meh. And half this is done in kernelspace for device add/replace (device size setting etc) so TBH I'm increasingly inclined to just back away slowly here. :(

(IOWs device_add calls btrfs_prepare_device(), but the size it finds is never used; the kernel does:

	device->total_bytes = i_size_read(bdev->bd_inode);

so changing prepare_device doesn't catch add/replace cases...)

Perhaps the simpler option is to remove the rounding which is only done on the first device added in userspace, but I honestly don't know what the design plan is, or what the ramifications of that might be ...

-Eric