Re: bvec_iter.bi_sector -> loff_t? (was: Re: [PATCH] bcachefs: allow direct io fallback to buffer io for) unaligned length or offset

Kent Overstreet
2024-06-21T03:52:29Z

On Fri, Jun 21, 2024 at 11:36:20AM +0800, Ming Lei wrote:
> On Thu, Jun 20, 2024 at 11:07:19PM -0400, Kent Overstreet wrote:
> > On Fri, Jun 21, 2024 at 09:48:11AM +0800, Ming Lei wrote:
> > > On Thu, Jun 20, 2024 at 11:43:44AM -0400, Kent Overstreet wrote:
> > > > On Thu, Jun 20, 2024 at 05:30:50PM +0200, Christoph Hellwig wrote:
> > > > > On Thu, Jun 20, 2024 at 02:54:09PM +0100, Matthew Wilcox wrote:
> > > > > > I'm against it. Block devices only do sector-aligned IO and we should
> > > > > > not pretend otherwise.
> > > > >
> > > > > While I agree with that, the bvec_iter is actually used in a few other
> > > > > places and could be used in more, and the 512-byte sector unit bi_sector
> > > > > is the only weird thing that's not useful elsewhere. So turning that
> > > > > into a
> > > > >
> > > > > u64 bi_addr;
> > > > >
> > > > > that is byte based where the meaning is specific to the user would
> > > > > actually be kinda nice. For traditional block users we'd need a
> > > > > bio_sector() helpers similar to the existing bio_sectors() one,
> > > > > but a lot of non-trivial drivers actually need to translated to
> > > > > a variable LBA-based addressing, which would be (a tiny little bit)
> > > > > simpler with the byte address. As bi_size is already in bytes
> > > > > it would also fit in pretty naturally with that.
> > > > >
> > > > > The only thing that is really off putting is the amount of churn that
> > > > > this would cause.
> > > >
> > > > I'm being imprecise when I just say 'struct bio'; there's things in
> > > > there that are block layer specific but there are also things in there
> > > > you want that aren't block layer specific (completion callback, write
> > > > flags, s/bi_bdev/bi_inode and that as well, perhaps). It's not at all
> > > > clear to me we'd want to deal with the churn to split that up or make
> > > > bio itself less block layer specific (although, but when I say 'aiming
> > > > for commality with struct bio' that sort of thing is what I have in
> > > > mind.
> > > >
> > > > But more immediately, yes - bi_addr as all we need for this, and like
> > > > you said I think it'd be a worthwhile change.
> > >
> > > Still not clear why you need unaligned bi_addr for bio, if this bio needs
> > > to call submit_bio(), it has to be aligned. Otherwise, you could invent any
> > > structure for this purpose, and the structure can be payload of bio for
> > > avoiding extra allocation, even it can be FS generic structure.
> >
> > We want to have fewer scatter/gather list data structures, not more.
>
> OK, that look fine to change to bi_addr since bvec_iter is widely used now,
> maybe .bi_sector can be moved into bio, cause bvec iterator needn't it.

No, we'd just add a bio_sector() helper that does the shift.