Re: [PATCH vfs.all 22/26] block: stash a bdev_file to read/write raw blcok_device

[Al Viro <viro@zeniv.linux.org.uk>]

On Sun, Apr 07, 2024 at 11:21:56AM +0800, Yu Kuai wrote:
> Hi,
> 
> 在 2024/04/07 11:06, Al Viro 写道:
> > On Sun, Apr 07, 2024 at 10:34:56AM +0800, Yu Kuai wrote:
> > 
> > > Other than raw block_device fops, other filesystems can use the opened
> > > bdev_file directly for iomap and buffer_head, and they actually don't
> > > need to reference block_device anymore. The point here is that whether
> > 
> > What do you mean, "reference"?  The counting reference is to opened
> > file; ->s_bdev is a cached pointer to associated struct block_device,
> > and neither it nor pointers in buffer_head are valid past the moment
> > when you close the file.  Storing (non-counting) pointers to struct
> > file in struct buffer_head is not different in that respect - they
> > are *still* only valid while the "master" reference is held.
> > 
> > Again, what's the point of storing struct file * in struct buffer_head
> > or struct iomap?  In any instances of those structures?
> 
> Perhaps this is what you missed, like the title of this set, in order to
> remove direct acceess of bdev->bd_inode from fs/buffer, we must store
> bdev_file in buffer_head and iomap, and 'bdev->bd_inode' is replaced
> with 'file_inode(bdev)' now.

BTW, what does that have to do with iomap?  All it passes ->bdev to is
	1) bio_alloc()
	2) bio_alloc_bioset()
	3) bio_init()
	4) bdev_logical_block_size()
	5) bdev_iter_is_aligned()
	6) bdev_fua() 
	7) bdev_write_cache()

None of those goes anywhere near fs/buffer.c or uses ->bd_inode, AFAICS.

Again, what's the point?  It feels like you are trying to replace *all*
uses of struct block_device with struct file, just because.

If that's what's going on, please don't.  Using struct file instead
of that bdev_handle crap - sure, makes perfect sense.  But shoving it
down into struct bio really, really does not.

I'd suggest to start with adding ->bd_mapping as the first step and
converting the places where mapping is all we want to using that.
Right at the beginning of your series.  Then let's see what gets
left.

And leave ->bd_inode there for now; don't blindly replace it with
->bd_mapping->host everywhere.  It's much easier to grep for.
The point of the exercise is to find what do we really need ->bd_inode
for and what primitives are missing, not getting rid of a bad word...