On Wed, Jun 29, 2022 at 02:07:08AM -0400, Mike Snitzer wrote: > Please try to dial down the hyperbole and judgment. Ming wrote this > code. And you haven't been able to point out anything _actually_ wrong > with it (yet). > > This patch's header does need editing for clarity, but we can help > improve it and the documentation above bio_rewind() in the code. > > > So, and I'm sorry I have to be the killjoy here, but hard NACK on this patchset. > > Hard, hard NACK. > > <insert tom-delonge-wtf.gif> > > You see this bio_rewind() as history repeating itself, but it isn't > like what you ranted about in the past: > https://marc.info/?l=linux-block&m=153549921116441&w=2 > > I can certainly see why you think it similar at first glance. But this > patchset shows how bio_rewind() must be used, and how DM benefits from > using it safely (with no impact to struct bio or DM's per-bio-data). > > bio_rewind() usage will be as niche as DM's use-case for it. If other > code respects the documented constraint, that the original bio's end > sector be preserved, then they can use it too. > > The key is for a driver to maintain enough state to allow this fixed > end be effectively immutable. (DM happens to get this state "for free" > simply because it was already established for its IO accounting of > split bios). > > The Linux codebase requires precision. This isn't new. Mike, that's not justification for making things _more_ dangerous. > > > I'll be happy to assist in coming up with alternate, less dangerous solutions > > though (and I think introducing a real bio_iter is overdue, so that's probably > > the first thing we should look at). > > It isn't dangerous. It is an interface whose constraint needs to be > respected. Just like is documented for a myriad other kernel > interfaces. > > Factoring out a bio_iter will bloat struct bio for functionality most > consumers don't need. And gating DM's ability to achieve this > patchset's functionality with some overdue refactoring is really _not_ > acceptable. Mike, you're the one who's getting seriously hyperbolic here. You're getting frustrated because you've got this one thing you really want to get done, and you feel like you're running into a brick wall when I tell you "no". And yes, coding in the kernel is a complicated, dangerous environment with many rules that need to be respected. That does not mean it's ok to be adding to that complexity, and making it even more dangerous, without a _really fucking good reason_. This doesn't fly. Maybe it would if it was some device mapper private thing, but you're acting like it's only going to be used by device mapper when you're trying to add it to the public interface for core block layer bio code. _That_ needs real justification. Also, bio_iter is something we should definitely be considering because of the way integrity and now crypt has been tacked on to struct bio. When I originally wrote the modern bvec_iter code, the ability to use an iterator besides the one in struct bio was an important piece of functionality, one that's still in use (including in device mapper; see __bio_for_each_segment()). The fact that we're growing additional data structures that in theory want to be iterated in lockstep with the main bio payload but _aren't_ iterated over with bi_iter is, at best, a code smell and a lurking footgun. However, I can see that the two of you are not likely take on figuring out how to clean that up, and truthfully I don't have the time right now either, much as it pains me. Here's an alternative approach: The fundamental problem with bio_rewind() (and I know that you two are super serious that this is completely safe for your use case and no one else is going to use it for anything else) is that we're using it to get back to some initial state, but it's not invariant w.r.t. what's been done to the bio since then, and the nature of the block layer is that that's a problem. So here's what you do: You bring back bi_done: bi_done counts bytes advanced, total, since the start of the bio. Then we introduce a type: struct bio_pos { unsigned bi_done; unsigned bi_size; }; And two new functions: struct bio_pos bio_get_pos(struct bio *) { ... } void bio_set_pos(struct bio *, struct bio_pos) { ... } That gets you the same functionality as bio_rewind(), but it'll be much more broadly useful.
