[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH -next] block: remove field 'bd_inode' from block_device



Hi,

在 2023/11/25 22:32, Greg KH 写道:
On Sat, Nov 25, 2023 at 05:39:12PM +0800, Yu Kuai wrote:
From: Yu Kuai <yukuai3@xxxxxxxxxx>

block_devcie is allocated from bdev_alloc() by bdev_alloc_inode(), and
currently block_device contains a pointer that point to the address of
inode, while such inode is allocated together:

bdev_alloc
  inode = new_inode()
   // inode is &bdev_inode->vfs_inode
  bdev = I_BDEV(inode)
   // bdev is &bdev_inode->bdev
  bdev->inode = inode

Add a new helper to get address of inode from bdev by add operation
instead of memory access, which is more efficiency. Also prepare to
add a new field 'bd_flags' in the first cacheline(64 bytes).

Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
---
  block/bdev.c                       | 39 +++++++++++++++++-------------
  block/blk-zoned.c                  |  4 +--
  block/fops.c                       |  4 +--
  block/genhd.c                      |  8 +++---
  block/ioctl.c                      |  8 +++---
  block/partitions/core.c            |  9 ++++---
  drivers/block/xen-blkback/xenbus.c |  2 +-
  drivers/md/bcache/super.c          |  2 +-
  drivers/mtd/devices/block2mtd.c    | 12 ++++-----
  drivers/s390/block/dasd_ioctl.c    |  2 +-
  drivers/scsi/scsicam.c             |  2 +-
  fs/bcachefs/util.h                 |  2 +-
  fs/btrfs/disk-io.c                 |  6 ++---
  fs/btrfs/volumes.c                 |  4 +--
  fs/btrfs/zoned.c                   |  2 +-
  fs/buffer.c                        |  8 +++---
  fs/cramfs/inode.c                  |  2 +-
  fs/erofs/data.c                    |  2 +-
  fs/ext4/dir.c                      |  2 +-
  fs/ext4/ext4_jbd2.c                |  2 +-
  fs/ext4/super.c                    |  8 +++---
  fs/gfs2/glock.c                    |  2 +-
  fs/gfs2/ops_fstype.c               |  2 +-
  fs/jbd2/journal.c                  |  3 ++-
  fs/jbd2/recovery.c                 |  2 +-
  fs/nilfs2/segment.c                |  2 +-
  include/linux/blk_types.h          | 10 ++++++--
  include/linux/blkdev.h             |  4 +--
  include/linux/buffer_head.h        |  4 +--
  29 files changed, 86 insertions(+), 73 deletions(-)

You should do this as a patch series, add the helper function that does
nothing, convert all the different portions of the kernel as different
patches, and _then_ change the implementation of the block layer to
handle the change in the structure.

Otherwise this is going to be hard to get accepted.

Okay, thanks for the adivce, I'll do that in v2.

By the way, I was thinking that this patch is quite simple, and doesn't
worth spliting into 10+ patches,

Also, one note:

@@ -85,6 +84,13 @@ struct block_device {
  #define bdev_kobj(_bdev) \
        (&((_bdev)->bd_device.kobj))
+static inline struct inode *bdev_inode(struct block_device *bdev)
+{
+       void *inode = bdev + 1;

That's crazy, if something changes, this will keep working yet the
kernel will break and no one will know why.

Please use container_of(), that's what it is there for, this exact type
of thing.  Or if not, are you just assuming that the memory location
right after bdev is the inode?  That's a tough assumption, how are you
going to assure it really stays there?

Struct bdev_inode never changes since commit 8fbd544cbca5 ("[PATCH]
bdev: add I_BDEV()") from 2004, and I think it won't change unless
there is a different way to manage lifetime of block_device.

And the 'bdev + 1' is copied from blk_mq_rq_to_pdu(), however, I aggre
that use container_of() is better and I will use it in v2.

Thanks,
Kuai


thanks,

greg k-h
.





 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.