| Message ID | f870c029-140d-3e77-dcd1-1890025b5795@kernel.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [GIT,PULL] bdev size cleanups | expand |
On Sun, Oct 31, 2021 at 12:41 PM Jens Axboe <axboe@kernel.dk> wrote: > > On top of the core block branch, this topic branch cleans up the bdev > size handling. So on the whole this seems to be a good cleanup, but some of it worries me. For example, it seems to have lost the cast to "loff_t" when generating the byte size from a "sector_t". Ok, so these days those are both 64-bit, and it doesn't actually matter (the time when we had a 32-bit sector_t as an option are long gone), but I think that bdev_nr_bytes() helper really ends up being subtler than it looks. It very much depends on 'sector_t' and 'loff_t' being the same size (although sector_t is an u64, loff_t ends up being the signed version). I've pulled this, but I do think it might have been better with the type conversion being explicit. One of the reasons we had "sector_t" originally was that it ended up being configuration-dependent, and could be 32-bit. Those times may be gone, but it's still conceptually a very different type from "loff_t". Linus
The pull request you sent on Sun, 31 Oct 2021 13:41:51 -0600:
> git://git.kernel.dk/linux-block.git tags/for-5.16/bdev-size-2021-10-29
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/3f01727f750eae3e61b738b57355b2538ab179f4
Thank you!
On 11/1/21 11:02 AM, Linus Torvalds wrote: > On Sun, Oct 31, 2021 at 12:41 PM Jens Axboe <axboe@kernel.dk> wrote: >> >> On top of the core block branch, this topic branch cleans up the bdev >> size handling. > > So on the whole this seems to be a good cleanup, but some of it worries me. > > For example, it seems to have lost the cast to "loff_t" when > generating the byte size from a "sector_t". > > Ok, so these days those are both 64-bit, and it doesn't actually > matter (the time when we had a 32-bit sector_t as an option are long > gone), but I think that bdev_nr_bytes() helper really ends up being > subtler than it looks. It very much depends on 'sector_t' and 'loff_t' > being the same size (although sector_t is an u64, loff_t ends up being > the signed version). > > I've pulled this, but I do think it might have been better with the > type conversion being explicit. One of the reasons we had "sector_t" > originally was that it ended up being configuration-dependent, and > could be 32-bit. Those times may be gone, but it's still conceptually > a very different type from "loff_t". Yes, probably safer just to make bdev_nr_bytes() return sector_t as well, even if loff_t isn't strictly wrong. Christoph, want to do a followup?
On Mon, Nov 1, 2021 at 4:20 PM Jens Axboe <axboe@kernel.dk> wrote: > > Yes, probably safer just to make bdev_nr_bytes() return sector_t as > well, even if loff_t isn't strictly wrong. Well, that would actually change the sign of some of the existing comparisons. Possibly changing their meaning entirely.. So having 'loff_t' being signed may be an odd choice for a byte size, but it is what it is. At least the current set of cleanups seemed to keep the type logic the same when it changed i_size_read() to be bdev_nr_bytes() instead. Changing it to 'sector_t' not only doesn't make conceptual sense when it's a byte count, it might also be dangerous. So my reaction was really that it wasn't obvious that bdev_nr_bytes() did the shift in the right type.. It does happen to do that, but historically sector_t was the smaller type. Linus
On Mon, Nov 01, 2021 at 05:19:58PM -0600, Jens Axboe wrote: > Yes, probably safer just to make bdev_nr_bytes() return sector_t as > well, even if loff_t isn't strictly wrong. Christoph, want to do a > followup? sector_t is wrong. I actually did that in the very first version accidentally and willy pointed out that this is wrong. If Linus really wants that we can add the cast.
On 11/1/21 5:50 PM, Linus Torvalds wrote: > On Mon, Nov 1, 2021 at 4:20 PM Jens Axboe <axboe@kernel.dk> wrote: >> >> Yes, probably safer just to make bdev_nr_bytes() return sector_t as >> well, even if loff_t isn't strictly wrong. > > Well, that would actually change the sign of some of the existing > comparisons. Possibly changing their meaning entirely.. > > So having 'loff_t' being signed may be an odd choice for a byte size, > but it is what it is. At least the current set of cleanups seemed to > keep the type logic the same when it changed i_size_read() to be > bdev_nr_bytes() instead. > > Changing it to 'sector_t' not only doesn't make conceptual sense when > it's a byte count, it might also be dangerous. > > So my reaction was really that it wasn't obvious that bdev_nr_bytes() > did the shift in the right type.. It does happen to do that, but > historically sector_t was the smaller type. OK, I misunderstood your original email, as per Christoph's email as well. May be worth adding loff_t cast, if for nothing else just to have it stick out to the next person touching it.
Hi Linus, On top of the core block branch, this topic branch cleans up the bdev size handling. Please pull! The following changes since commit 4f5022453acd0f7b28012e20b7d048470f129894: nvme: wire up completion batching for the IRQ path (2021-10-18 14:40:47 -0600) are available in the Git repository at: git://git.kernel.dk/linux-block.git tags/for-5.16/bdev-size-2021-10-29 for you to fetch changes up to 97eeb5fc14cc4b2091df8b841a07a1ac69f2d762: partitions/ibm: use bdev_nr_sectors instead of open coding it (2021-10-19 06:17:33 -0600) ---------------------------------------------------------------- for-5.16/bdev-size-2021-10-29 ---------------------------------------------------------------- Christoph Hellwig (33): block: move the SECTOR_SIZE related definitions to blk_types.h block: add a bdev_nr_bytes helper bcache: remove bdev_sectors drbd: use bdev_nr_sectors instead of open coding it dm: use bdev_nr_sectors and bdev_nr_bytes instead of open coding them md: use bdev_nr_sectors instead of open coding it nvmet: use bdev_nr_bytes instead of open coding it target/iblock: use bdev_nr_bytes instead of open coding it fs: use bdev_nr_bytes instead of open coding it in blkdev_max_block fs: simplify init_page_buffers affs: use bdev_nr_sectors instead of open coding it btrfs: use bdev_nr_bytes instead of open coding it cramfs: use bdev_nr_bytes instead of open coding it fat: use bdev_nr_sectors instead of open coding it hfs: use bdev_nr_sectors instead of open coding it hfsplus: use bdev_nr_sectors instead of open coding it jfs: use bdev_nr_bytes instead of open coding it nfs/blocklayout: use bdev_nr_bytes instead of open coding it nilfs2: use bdev_nr_bytes instead of open coding it ntfs3: use bdev_nr_bytes instead of open coding it pstore/blk: use bdev_nr_bytes instead of open coding it reiserfs: use bdev_nr_bytes instead of open coding it squashfs: use bdev_nr_bytes instead of open coding it block: use bdev_nr_bytes instead of open coding it in blkdev_fallocate block: add a sb_bdev_nr_blocks helper ext4: use sb_bdev_nr_blocks jfs: use sb_bdev_nr_blocks ntfs: use sb_bdev_nr_blocks reiserfs: use sb_bdev_nr_blocks udf: use sb_bdev_nr_blocks block/ioctl: use bdev_nr_sectors and bdev_nr_bytes partitions/efi: use bdev_nr_bytes instead of open coding it partitions/ibm: use bdev_nr_sectors instead of open coding it Jens Axboe (1): block: cache inode size in bdev block/fops.c | 2 +- block/genhd.c | 1 + block/ioctl.c | 20 ++++++++------------ block/partitions/core.c | 1 + block/partitions/efi.c | 2 +- block/partitions/ibm.c | 19 ++++++++++--------- drivers/block/drbd/drbd_int.h | 3 +-- drivers/md/bcache/super.c | 2 +- drivers/md/bcache/util.h | 4 ---- drivers/md/bcache/writeback.c | 2 +- drivers/md/dm-bufio.c | 2 +- drivers/md/dm-cache-metadata.c | 2 +- drivers/md/dm-cache-target.c | 2 +- drivers/md/dm-clone-target.c | 2 +- drivers/md/dm-dust.c | 5 ++--- drivers/md/dm-ebs-target.c | 2 +- drivers/md/dm-era-target.c | 2 +- drivers/md/dm-exception-store.h | 2 +- drivers/md/dm-flakey.c | 3 +-- drivers/md/dm-integrity.c | 6 +++--- drivers/md/dm-linear.c | 3 +-- drivers/md/dm-log-writes.c | 4 ++-- drivers/md/dm-log.c | 2 +- drivers/md/dm-mpath.c | 2 +- drivers/md/dm-raid.c | 6 +++--- drivers/md/dm-switch.c | 2 +- drivers/md/dm-table.c | 3 +-- drivers/md/dm-thin-metadata.c | 2 +- drivers/md/dm-thin.c | 2 +- drivers/md/dm-verity-target.c | 3 +-- drivers/md/dm-writecache.c | 2 +- drivers/md/dm-zoned-target.c | 2 +- drivers/md/md.c | 26 +++++++++++--------------- drivers/nvme/target/io-cmd-bdev.c | 4 ++-- drivers/target/target_core_iblock.c | 4 ++-- fs/affs/super.c | 2 +- fs/btrfs/dev-replace.c | 3 +-- fs/btrfs/disk-io.c | 2 +- fs/btrfs/ioctl.c | 4 ++-- fs/btrfs/volumes.c | 8 ++++---- fs/buffer.c | 4 ++-- fs/cramfs/inode.c | 2 +- fs/ext4/super.c | 2 +- fs/fat/inode.c | 5 +---- fs/hfs/mdb.c | 2 +- fs/hfsplus/wrapper.c | 2 +- fs/jfs/resize.c | 5 ++--- fs/jfs/super.c | 5 ++--- fs/nfs/blocklayout/dev.c | 4 ++-- fs/nilfs2/ioctl.c | 2 +- fs/nilfs2/super.c | 2 +- fs/nilfs2/the_nilfs.c | 2 +- fs/ntfs/super.c | 8 +++----- fs/ntfs3/super.c | 2 +- fs/pstore/blk.c | 8 +++----- fs/reiserfs/super.c | 8 ++------ fs/squashfs/super.c | 5 +++-- fs/udf/lowlevel.c | 5 ++--- fs/udf/super.c | 9 +++------ include/linux/blk_types.h | 18 ++++++++++++++++++ include/linux/blkdev.h | 17 ----------------- include/linux/genhd.h | 13 ++++++++++++- 62 files changed, 140 insertions(+), 160 deletions(-)