On Wed, Jun 29, 2022 at 02:11:54PM -0400, Kent Overstreet wrote: > 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. What is the difference between bio_set_pos and bio_rewind()? Both have to restore bio->bi_iter(the sector part and the bvec part). Also how to update ->bi_done which 'counts bytes advanced'? You meant doing it in very bio_advance()? then no, why do we have to pay the cost for very unusual bio_rewind()? Or if I misunderstood your point, please cook a patch and I am happy to take a close look, and posting one very raw idea with random data structure looks not helpful much for this discussion technically. Thanks, Ming
