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