On Mon, Apr 15, 2024 at 09:45:11PM +0100, Al Viro wrote: > On Sat, Apr 13, 2024 at 05:25:01PM +0200, Christian Brauner wrote: > > > > It also simplifies the hell out of the patch series - it's one obviously > > > safe automatic change in a single commit. > > > > It's trivial to fold the simple file_mapping() conversion into a single > > patch as well. > > ... after a bunch of patches that propagate struct file to places where > it has no business being. Compared to a variant that doesn't need those > patches at all. > > > It's a pure artifact of splitting the patches per > > subsystem/driver. > > No, it is not. ->bd_mapping conversion can be done without any > preliminaries. Note that it doesn't need messing with bdev_read_folio(), > it doesn't need this journal->j_fs_dev_file thing, etc. > > One thing I believe is completely wrong in this series is bdev_inode() > existence. It (and equivalent use of file_inode() on struct file is > even worse) is papering over the real interface deficiencies. And > extra file_inode() uses are just about impossible to catch ;-/ > > IMO we should *never* use file_inode() on opened block devices. > At all. It's brittle, it's asking for trouble as soon as somebody > passes a normally opened struct file to one of the functions using it > and it papers over the missing primitives. BTW, speaking of the things where opened struct file would be a good idea - set_blocksize() should take an opened struct file, and it should have non-NULL ->private_data. Changing block size under e.g. a mounted filesystem should never happen; doing that is asking for serious breakage. Looking through the current callers (mainline), most are OK (and easy to switch). However, drivers/block/pktcdvd.c:2285: set_blocksize(disk->part0, CD_FRAMESIZE); drivers/block/pktcdvd.c:2529: set_blocksize(file_bdev(bdev_file), CD_FRAMESIZE); Might be broken; pktcdvd.c being what it is... drivers/md/bcache/super.c:2558: if (set_blocksize(file_bdev(bdev_file), 4096)) Almost certainly broken; hit register_bcache() with pathname of a mounted block device, and if the block size on filesystem in question is not 4K, the things will get interesting. fs/btrfs/volumes.c:485: ret = set_blocksize(bdev, BTRFS_BDEV_BLOCKSIZE); Some of the callers do not bother with exclusive open; in particular, if btrfs_get_dev_args_from_path() ever gets a pathname of a mounted device with something other than btrfs on it, it won't be pretty. kernel/power/swap.c:371: res = set_blocksize(file_bdev(hib_resume_bdev_file), PAGE_SIZE); kernel/power/swap.c:1577: set_blocksize(file_bdev(hib_resume_bdev_file), PAGE_SIZE); Special cases (for obvious reasons); said that, why do we bother with set_blocksize() on those anyway?
