Re: [PATCH] Consolidate bio_clone_bioset(), bio_kmalloc()
On Wed, Aug 08, 2012 at 07:38:11PM -0700, Kent Overstreet wrote: > So here's my initial stab at it. Tell me if you think this is too > contorted: At the first glance, looks okay to me. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] Consolidate bio_clone_bioset(), bio_kmalloc()
On Wed, Aug 08, 2012 at 07:38:11PM -0700, Kent Overstreet wrote: So here's my initial stab at it. Tell me if you think this is too contorted: At the first glance, looks okay to me. Thanks. -- tejun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Consolidate bio_clone_bioset(), bio_kmalloc()
On Wed, Aug 08, 2012 at 04:15:52PM -0700, Tejun Heo wrote:
> > +struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask)
> > +{
> > + struct bio *b = bio_kmalloc(gfp_mask, bio->bi_max_vecs);
>
> Can't we use %NULL bioset as an indication to allocate from kmalloc
> instead of duping interfaces like this?
So, if bio_clone_bioset(gfp, nr_iovecs, BIO_KMALLOC_POOL) just does
bio_kmalloc(), the rest just falls out naturally.
We could do this by either just having bio_clone_bioset() call
bio_kmalloc(), or consolidate them both into a single function.
I'm leaning towards the latter, because while looking at it I noticed a
couple subtle behavioural differences.
* bio_kmalloc(GFP_KERNEL, 0) sets bi_io_vec = bi_inline_vecs,
bio_alloc_bioset sets it to NULL. This is a bug waiting to happen, if it
isn't one already - bi_io_vec != NULL is exactly what bio_has_data()
checks.
* bio_alloc_bioset() could return a bio with bi_max_vecs greater than
requested if you asked for a bio with fewer than BIO_INLINE_VECS.
Unlikely to ever be a real problem, but subtle enough that I wouldn't
bet too much against it.
* bio_kmalloc() fails if asked for more than UIO_MAXIOV bvecs (wtf!?),
which is 1024; bio_alloc_bioset fails if asked for more than
BIO_MAX_PAGES (which is 256, and it'd probably take you a bit to see
where/why it fails).
So here's my initial stab at it. Tell me if you think this is too
contorted:
diff --git a/fs/bio.c b/fs/bio.c
index 22596af..c852665 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -295,34 +295,45 @@ EXPORT_SYMBOL(bio_reset);
**/
struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
{
+ unsigned front_pad;
+ unsigned inline_vecs;
unsigned long idx = BIO_POOL_NONE;
struct bio_vec *bvl = NULL;
struct bio *bio;
void *p;
- p = mempool_alloc(bs->bio_pool, gfp_mask);
+ if (nr_iovecs > UIO_MAXIOV)
+ return NULL;
+
+ if (bs == BIO_KMALLOC_POOL) {
+ p = kmalloc(sizeof(struct bio) +
+ nr_iovecs * sizeof(struct bio_vec),
+ gfp_mask);
+ front_pad = 0;
+ inline_vecs = nr_iovecs;
+ } else {
+ p = mempool_alloc(bs->bio_pool, gfp_mask);
+ front_pad = bs->front_pad;
+ inline_vecs = BIO_INLINE_VECS;
+ }
+
if (unlikely(!p))
return NULL;
- bio = p + bs->front_pad;
+ bio = p + front_pad;
bio_init(bio);
- bio->bi_pool = bs;
-
- if (unlikely(!nr_iovecs))
- goto out_set;
- if (nr_iovecs <= BIO_INLINE_VECS) {
- bvl = bio->bi_inline_vecs;
- nr_iovecs = BIO_INLINE_VECS;
- } else {
+ if (nr_iovecs > inline_vecs) {
bvl = bvec_alloc_bs(gfp_mask, nr_iovecs, , bs);
if (unlikely(!bvl))
goto err_free;
- nr_iovecs = bvec_nr_vecs(idx);
bio->bi_flags |= 1 << BIO_OWNS_VEC;
+ } else if (nr_iovecs) {
+ bvl = bio->bi_inline_vecs;
}
-out_set:
+
+ bio->bi_pool = bs;
bio->bi_flags |= idx << BIO_POOL_OFFSET;
bio->bi_max_vecs = nr_iovecs;
bio->bi_io_vec = bvl;
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] Consolidate bio_clone_bioset(), bio_kmalloc()
On Wed, Aug 08, 2012 at 04:15:52PM -0700, Tejun Heo wrote:
+struct bio *bio_clone_kmalloc(struct bio *bio, gfp_t gfp_mask)
+{
+ struct bio *b = bio_kmalloc(gfp_mask, bio-bi_max_vecs);
Can't we use %NULL bioset as an indication to allocate from kmalloc
instead of duping interfaces like this?
So, if bio_clone_bioset(gfp, nr_iovecs, BIO_KMALLOC_POOL) just does
bio_kmalloc(), the rest just falls out naturally.
We could do this by either just having bio_clone_bioset() call
bio_kmalloc(), or consolidate them both into a single function.
I'm leaning towards the latter, because while looking at it I noticed a
couple subtle behavioural differences.
* bio_kmalloc(GFP_KERNEL, 0) sets bi_io_vec = bi_inline_vecs,
bio_alloc_bioset sets it to NULL. This is a bug waiting to happen, if it
isn't one already - bi_io_vec != NULL is exactly what bio_has_data()
checks.
* bio_alloc_bioset() could return a bio with bi_max_vecs greater than
requested if you asked for a bio with fewer than BIO_INLINE_VECS.
Unlikely to ever be a real problem, but subtle enough that I wouldn't
bet too much against it.
* bio_kmalloc() fails if asked for more than UIO_MAXIOV bvecs (wtf!?),
which is 1024; bio_alloc_bioset fails if asked for more than
BIO_MAX_PAGES (which is 256, and it'd probably take you a bit to see
where/why it fails).
So here's my initial stab at it. Tell me if you think this is too
contorted:
diff --git a/fs/bio.c b/fs/bio.c
index 22596af..c852665 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -295,34 +295,45 @@ EXPORT_SYMBOL(bio_reset);
**/
struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
{
+ unsigned front_pad;
+ unsigned inline_vecs;
unsigned long idx = BIO_POOL_NONE;
struct bio_vec *bvl = NULL;
struct bio *bio;
void *p;
- p = mempool_alloc(bs-bio_pool, gfp_mask);
+ if (nr_iovecs UIO_MAXIOV)
+ return NULL;
+
+ if (bs == BIO_KMALLOC_POOL) {
+ p = kmalloc(sizeof(struct bio) +
+ nr_iovecs * sizeof(struct bio_vec),
+ gfp_mask);
+ front_pad = 0;
+ inline_vecs = nr_iovecs;
+ } else {
+ p = mempool_alloc(bs-bio_pool, gfp_mask);
+ front_pad = bs-front_pad;
+ inline_vecs = BIO_INLINE_VECS;
+ }
+
if (unlikely(!p))
return NULL;
- bio = p + bs-front_pad;
+ bio = p + front_pad;
bio_init(bio);
- bio-bi_pool = bs;
-
- if (unlikely(!nr_iovecs))
- goto out_set;
- if (nr_iovecs = BIO_INLINE_VECS) {
- bvl = bio-bi_inline_vecs;
- nr_iovecs = BIO_INLINE_VECS;
- } else {
+ if (nr_iovecs inline_vecs) {
bvl = bvec_alloc_bs(gfp_mask, nr_iovecs, idx, bs);
if (unlikely(!bvl))
goto err_free;
- nr_iovecs = bvec_nr_vecs(idx);
bio-bi_flags |= 1 BIO_OWNS_VEC;
+ } else if (nr_iovecs) {
+ bvl = bio-bi_inline_vecs;
}
-out_set:
+
+ bio-bi_pool = bs;
bio-bi_flags |= idx BIO_POOL_OFFSET;
bio-bi_max_vecs = nr_iovecs;
bio-bi_io_vec = bvl;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
