On Tue 16-04-24 07:32:53, Al Viro wrote: > 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. Agreed. Furthermore that set_blocksize() seems to be completely pointless these days AFAICT because we use read_cache_page_gfp() to read in the data from the device. Sure we may be creating more bhs per page than necessary but who cares? > 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. Yeah and frankly reading through btrfs_read_dev_super() I'm not sure which code needs the block size set either. We use read_cache_page_gfp() for the IO there as well. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
