On Tue 09-04-24 14:22:37, Yu Kuai wrote: > Hi, > > 在 2024/04/09 12:26, Al Viro 写道: > > 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. > > Thanks so much for your advice, in fact, I totally agree with this that > adding a 'bd_mapping' or expose the helper bdev_mapping(). > > However, I will let Christoph and Jan to make the decision, when they > get time to take a look at this. I agree with Christian and Al - and I think I've expressed that already in the previous version of the series [1] but I guess I was not explicit enough :). I think the initial part of the series (upto patch 21, perhaps excluding patch 20) is a nice cleanup but the latter part playing with stashing struct file is not an improvement and seems pointless to me. So I'd separate the initial part cleaning up the obvious places and let Christian merge it and then we can figure out what (if anything) to do with remaining bd_inode uses in fs/buffer.c etc. E.g. what Al suggests with bd_mapping makes sense to me but I didn't check what's left after your initial patches... Honza [1] https://lore.kernel.org/all/20240322125750.jov4f3alsrkmqnq7@quack3 -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
