Messages in this thread | | | Date | Thu, 9 Aug 2012 02:59:31 -0400 | From | Kent Overstreet <> | Subject | Re: [PATCH v5 11/12] block: Add bio_clone_bioset() |
| |
On Wed, Aug 08, 2012 at 11:52:51PM -0700, Tejun Heo wrote: > On Wed, Aug 08, 2012 at 07:56:10PM -0700, Kent Overstreet wrote: > > > So, bio_clone() loses its function comment. Also, does it even make > > > sense to call bio_clone() from fs_bio_set? > > > > I'll re add the function comment if you want, just for a single line > > wrapper I don't know if it's worth the cost - comments get out of date, > > and they're more stuff to wade through. > > People actually look at docbook generated docs. I don't know why but > they do. It's a utility function at block layer. Please just add the > comment.
Will do then.
> > > Let's say it's so, then > > > what's the difference from using _kmalloc variant? > > > > bio_kmalloc() fails if nr_iovecs > 1024, bio_alloc_bioset() fails if > > nr_iovecs > 256 > > > > and bio_alloc_bioset() is mempool backed, bio_kmalloc() is not. > > > > AFAICT that's it. > > So, the thing is being mempool backed doesn't mean anything if > multiple layers use the pool.
It's worse than just using kmalloc, because then you've introduced the possibility of deadlock.
> I *suspect* fs_bio_set is supposed to > be used by fs layer - ie. where bios originate. The reason why I > wondered about bio_clone() is that bio_clone() is almost always used > from stacking drivers and stacking driver tapping into fs reserve is > buggy. So, I'm wondering whether cloning from fs_bio_set should be > supported at all.
That's actually a really good point.
I just grepped and there's actually only 3 callers - I thought there'd be more. That should be easy to fix, at least.
| |