On Tue, Oct 25, 2016 at 06:34:43PM +0200, Christoph Hellwig wrote: > On Tue, Oct 25, 2016 at 07:31:56AM -0800, Kent Overstreet wrote: > Yes, this is the basic idea behind the whole iomap code - we have > a generic apply function that calls into the fs allocator, and then > takes a callback that it applies to one extent at a time. It really > helps a lot to separate responsibilities and factor the code into > independent functions. > > I looked at carrying over bios between different invocations of the > callback, but it quickly turned into a mess. Really? Ages ago that was the approach I was taking with my dio rewrite and it worked out fine - it was using get_block but I'd think the code would look almost exactly the same with iomap. Anyways I'm not at all advocating you redo your code - if it works, please ship this so direct-IO.c can die in a fire! - but I would encourage you to take a look at my code, I think there's some good ideas in there. I think my (ancient, and I never got it 100% debugged) dio rewrite is easiest to follow - maybe you've already seen it if you found my bio_get_user_pages() code, but if you haven't: https://evilpiepirate.org/git/linux-bcache.git/tree/fs/direct-io.c?h=dio getting on a bit of a tangent now: the actor approach you're taking with iomap_apply() really reminds me of going through internal vs. external iterators in bcache. We started out with internal iterators - which is what's still in the upstream code, bch_btree_map_keys() is roughly equivalent to iomap_apply() where you pass it a callback and it does the iteration. But later we ended up switching to external iterators - we've now got a struct btree_iter, with functions for init/peek/advance/set_pos, and inserting at an iterator's current position. It was actually another guy who convinced me to consider switching to external iterators (Slava Pestov), and it was quite a pain in the ass (walking a btree directly is a whole lot easier than implementing a state machine to do it, partly) - but it was _hugely_ worth it in the long run because it they're a whole lot more flexible to use and they made a lot of things possible that honestly I wouldn't have thought of before I had them. Don't know that it's actually relevant here - I haven't dug into your iomap code yet, just lately I've been noticing more places in the kernel where people have implemented stuff with internal iterators (e.g. the crypto code) or no real iterator at all for things where I'm pretty sure an external iterator could make things a whole lot simpler. > git://git.infradead.org/users/hch/vfs.git iomap-dio Cool, reading. Also - what are you doing about the race between shooting down the range in the pagecache and dirty pages being readded? The existing direct IO code falls back to buffered IO for that, but your code doesn't appear to - I seem to recall that XFS has its own locking for this, are you just relying on that for now? It'd be really nice to get some generic locking for this, anything that relies on pagecache invalidation is sketchy as hell in other filesystems. -- To unsubscribe from this list: send the line "unsubscribe linux-block" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
