On Fri, Apr 12, 2024 at 12:29:19PM +0100, Al Viro wrote: > On Fri, Apr 12, 2024 at 11:21:08AM +0200, Christian Brauner wrote: > > > Your series just replaces bd_inode in struct block_device with > > bd_mapping. In a lot of places we do have immediate access to the bdev > > file without changing any calling conventions whatsoever. IMO it's > > perfectly fine to just use file_mapping() there. Sure, let's use > > bdev_mapping() in instances like btrfs where we'd otherwise have to > > change function signatures I'm not opposed to that. But there's no good > > reason to just replace everything with bdev->bd_mapping access. And > > really, why keep that thing in struct block_device when we can avoid it. > > Because having to have struct file around in the places where we want to > get to page cache of block device fast is often inconvenient (see fs/buffer.c, > if nothing else). Yes, agreed. But my point is why can't we expose bdev_mapping() for exactly that purpose without having to have that bd_mapping member in struct block_device? We don't want to trade bd_inode for bd_mapping in that struct imho. IOW, if we can avoid bloating struct block device with additional members then we should do that. Is there some performance concern that I'm missing and if so are there numbers to back this? > 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. It's a pure artifact of splitting the patches per subsystem/driver. That's just because people have wildly different opinions on how to do such conversion. But really, that can be trivially dealt with. > And AFAICS the flags-related rationale can be dealt with in a much simpler > way - see #bf_flags in my tree. That's certainly worth doing independent of this discussion.
