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:

> 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.

TBH, that looks like a very massive overkill that doesn't address
the real issues - you are still poking in that coallocated struct
inode, only instead of fetching it from pointer in struct block_device
you get it from a pointer in a dummy struct file.  How is that an
improvement?  After all, grepping for '\->[ 	]*bd_inode\>' and
looking through the few remaining users in e.g. fs/buffer.c is
*much* easier than grepping for file_inode callers.

AFAICS, Christoph's objections had been about the need to use saner
APIs instead of getting to inode in some way and poking in it.
And I agree that quite a few things in that series do just
that.  The final part doesn't.

IMO, those dummy struct file (used as convenient storage for pointer
to address_space *and* to the damn inode, with all its guts hanging
out) are simply wrong.

To reiterate:
	* we need to reduce the number of uses of those inodes
	* we need to find out what *is* getting used and sort out
the sane set of primitives; that's hard to do when we still have
a lot of noise.
	* we need convert to those saner primitives
	* we need to prevent reintroduction of noise, or at least
make such reintroduced noise easy to catch and whack.

->bd_inode is a problem because it's an attractive nuisance.  Removing
it would be fine, if there wasn't a harder to spot alternative way to
get the same pointer.  Try to grep for file_inode and bd_inode resp.
and compare the hit counts.  Seriously reduced set of bd_inode users is
fine - my impression is that after this series without the final part
we'd be down to 20 users or so.  In the meanwhile, there's about 1.4e3
users of file_inode(), most of them completely unrelated to block
devices.

PS: in grow_dev_folio() we probably want
	struct address_space *mapping = bdev->bd_inode->i_mapping;
instead of
	struct inode *inode = bdev->bd_inode;
as one of the preliminary chunks.
FWIW, it really looks like address_space (== page cache of block device,
not an unreasonably candidate for primitive) and block size (well,
logarithm thereof) cover the majority of what remains, with device
size possibly being (remote) third...