FAQ Search Today's Posts Mark Forums Read
» Video Reviews

» Linux Archive

Linux-archive is a website aiming to archive linux email lists and to make them easily accessible for linux users/developers.


» Sponsor

» Partners

» Sponsor

Go Back   Linux Archive > Redhat > Device-mapper Development

 
 
LinkBack Thread Tools
 
Old 08-28-2012, 08:31 PM
Tejun Heo
 
Default block: Add bio_reset()

Hello, Kent.

On Tue, Aug 28, 2012 at 10:37:30AM -0700, Kent Overstreet wrote:
> Reusing bios is something that's been highly frowned upon in the past,
> but driver code keeps doing it anyways. If it's going to happen anyways,
> we should provide a generic method.
>
> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> was open coding it, by doing a bio_init() and resetting bi_destructor.

Better to explain why some bio fields are re-ordered and why that
shouldn't make things worse cacheline-wise?

> +void bio_reset(struct bio *bio)
> +{

Function comment explaining what it does and why it does what it does
with integrity / bi_css / whatnot?

> + unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
> +
> + if (bio_integrity(bio))
> + bio_integrity_free(bio, bio->bi_pool);
> +
> + bio_disassociate_task(bio);

Is this desirable? Why?

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-28-2012, 10:17 PM
Kent Overstreet
 
Default block: Add bio_reset()

On Tue, Aug 28, 2012 at 01:31:48PM -0700, Tejun Heo wrote:
> Hello, Kent.
>
> On Tue, Aug 28, 2012 at 10:37:30AM -0700, Kent Overstreet wrote:
> > Reusing bios is something that's been highly frowned upon in the past,
> > but driver code keeps doing it anyways. If it's going to happen anyways,
> > we should provide a generic method.
> >
> > This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> > was open coding it, by doing a bio_init() and resetting bi_destructor.
>
> Better to explain why some bio fields are re-ordered and why that
> shouldn't make things worse cacheline-wise?

Well it may (struct bio is what, 3 or 4 cachelines now?) but even on
ridiculous million iop devices struct bio just isn't hot enough for this
kind of stuff to show up in the profiles... and I've done enough
profiling to be pretty confident of that.

So um, if there was anything to say performance wise I would, but to me
it seems more that there isn't.

(pruning struct bio would have more of an effect performance wise, which
you know is something I'd like to do.)

>
> > +void bio_reset(struct bio *bio)
> > +{
>
> Function comment explaining what it does and why it does what it does
> with integrity / bi_css / whatnot?

Yeah, good idea.

>
> > + unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
> > +
> > + if (bio_integrity(bio))
> > + bio_integrity_free(bio, bio->bi_pool);
> > +
> > + bio_disassociate_task(bio);
>
> Is this desirable? Why?

*twitch* I should've thought more when I made that change.

It occurs to me now that we specifically talked about that and decided
to do it the other way - when I changed that I was just looking at
bio_free() and decided they needed to be symmetrical.

I still think they should be symmetrical, but if that's true bi_ioc and
bi_css need to be moved, and also bio_disassociate_task() should be
getting called from bio_free(), not bio_put().

Were you the one that added that call? I know you've been working on
that area of the code recently. Sticking it in bio_put() instead of
bio_free() seems odd to be, and they're completely equivalent now that
bio_free() is only called from bio_put() (save one instance I should
probably fix).

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-28-2012, 10:53 PM
Kent Overstreet
 
Default block: Add bio_reset()

On Tue, Aug 28, 2012 at 03:17:15PM -0700, Kent Overstreet wrote:
> On Tue, Aug 28, 2012 at 01:31:48PM -0700, Tejun Heo wrote:
> > > + unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
> > > +
> > > + if (bio_integrity(bio))
> > > + bio_integrity_free(bio, bio->bi_pool);
> > > +
> > > + bio_disassociate_task(bio);
> >
> > Is this desirable? Why?
>
> *twitch* I should've thought more when I made that change.
>
> It occurs to me now that we specifically talked about that and decided
> to do it the other way - when I changed that I was just looking at
> bio_free() and decided they needed to be symmetrical.
>
> I still think they should be symmetrical, but if that's true bi_ioc and
> bi_css need to be moved, and also bio_disassociate_task() should be
> getting called from bio_free(), not bio_put().

Though - looking at it again I didn't actually screw anything up when I
made that change, it's just bad style.

Just screwing around a bit with the patch below, but - couple thoughts:

1) I hate duplicated code, and for the stuff in
bio_init()/bio_free()/bio_reset() it's perhaps not worth it, it is the
kind of stuff that gets out of sync.

2) It'd be better to have bio_reset() reset as much as possible, i.e. be
as close to bio_init() as posible. Fewer differences to confuse people.


diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index 7c8fe1d..a38bc7d 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -148,6 +148,9 @@ void bio_integrity_free(struct bio *bio, struct bio_set *bs)

BUG_ON(bip == NULL);

+ if (!bs)
+ bs = fs_bio_set;
+
/* A cloned bio doesn't own the integrity metadata */
if (!bio_flagged(bio, BIO_CLONED) && !bio_flagged(bio, BIO_FS_INTEGRITY)
&& bip->bip_buf != NULL)
diff --git a/fs/bio.c b/fs/bio.c
index c938f42..56d8fa2 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -234,39 +234,47 @@ fallback:
return bvl;
}

+static void bio_free_stuff(struct bio *bio)
+{
+ bio_disassociate_task(bio);
+
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, bio->bi_pool);
+}
+
void bio_free(struct bio *bio)
{
struct bio_set *bs = bio->bi_pool;
void *p;

+ bio_free_stuff(bio);
+
if (!bs) {
- /* Bio was allocated by bio_kmalloc() */
- if (bio_integrity(bio))
- bio_integrity_free(bio, fs_bio_set);
kfree(bio);
- return;
- }
-
- if (bio_has_allocated_vec(bio))
- bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
+ } else {
+ if (bio_has_allocated_vec(bio))
+ bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));

- if (bio_integrity(bio))
- bio_integrity_free(bio, bs);
+ /*
+ * If we have front padding, adjust the bio pointer before freeing
+ */
+ p = bio;
+ if (bs->front_pad)
+ p -= bs->front_pad;

- /*
- * If we have front padding, adjust the bio pointer before freeing
- */
- p = bio;
- if (bs->front_pad)
- p -= bs->front_pad;
+ mempool_free(p, bs->bio_pool);
+ }
+}

- mempool_free(p, bs->bio_pool);
+static void __bio_init(struct bio *bio)
+{
+ memset(bio, 0, BIO_RESET_BYTES);
+ bio->bi_flags = 1 << BIO_UPTODATE;
}

void bio_init(struct bio *bio)
{
- memset(bio, 0, sizeof(*bio));
- bio->bi_flags = 1 << BIO_UPTODATE;
+ __bio_init(bio);
atomic_set(&bio->bi_cnt, 1);
}
EXPORT_SYMBOL(bio_init);
@@ -275,13 +283,10 @@ void bio_reset(struct bio *bio)
{
unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);

- if (bio_integrity(bio))
- bio_integrity_free(bio, bio->bi_pool);
+ bio_free_stuff(bio);
+ __bio_init(bio);

- bio_disassociate_task(bio);
-
- memset(bio, 0, BIO_RESET_BYTES);
- bio->bi_flags = flags|(1 << BIO_UPTODATE);
+ bio->bi_flags |= flags;
}
EXPORT_SYMBOL(bio_reset);

@@ -440,10 +445,8 @@ void bio_put(struct bio *bio)
/*
* last put frees it
*/
- if (atomic_dec_and_test(&bio->bi_cnt)) {
- bio_disassociate_task(bio);
+ if (atomic_dec_and_test(&bio->bi_cnt))
bio_free(bio);
- }
}
EXPORT_SYMBOL(bio_put);

diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index ca63847..28bddc0 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -68,15 +68,6 @@ struct bio {

struct bvec_iter bi_iter;

- /*
- * Everything starting with bi_max_vecs will be preserved by bio_reset()
- */
-
- unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
-
- atomic_t bi_cnt; /* pin count */
-
- struct bio_vec *bi_io_vec; /* the actual vec list */
#ifdef CONFIG_BLK_CGROUP
/*
* Optional ioc and css associated with this bio. Put on bio
@@ -89,6 +80,16 @@ struct bio {
struct bio_integrity_payload *bi_integrity; /* data integrity */
#endif

+ /*
+ * Everything starting with bi_max_vecs will be preserved by bio_reset()
+ */
+
+ unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
+
+ atomic_t bi_cnt; /* pin count */
+
+ struct bio_vec *bi_io_vec; /* the actual vec list */
+
struct bio_set *bi_pool;

/*

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-01-2012, 02:23 AM
Tejun Heo
 
Default block: Add bio_reset()

Hello,

On Tue, Aug 28, 2012 at 03:17:15PM -0700, Kent Overstreet wrote:
> > Better to explain why some bio fields are re-ordered and why that
> > shouldn't make things worse cacheline-wise?
>
> Well it may (struct bio is what, 3 or 4 cachelines now?) but even on
> ridiculous million iop devices struct bio just isn't hot enough for this
> kind of stuff to show up in the profiles... and I've done enough
> profiling to be pretty confident of that.
>
> So um, if there was anything to say performance wise I would, but to me
> it seems more that there isn't.
>
> (pruning struct bio would have more of an effect performance wise, which
> you know is something I'd like to do.)

Yeah, please put something like the above in the patch description.

> > > + unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
> > > +
> > > + if (bio_integrity(bio))
> > > + bio_integrity_free(bio, bio->bi_pool);
> > > +
> > > + bio_disassociate_task(bio);
> >
> > Is this desirable? Why?
>
> *twitch* I should've thought more when I made that change.
>
> It occurs to me now that we specifically talked about that and decided
> to do it the other way - when I changed that I was just looking at
> bio_free() and decided they needed to be symmetrical.
>
> I still think they should be symmetrical, but if that's true bi_ioc and
> bi_css need to be moved, and also bio_disassociate_task() should be
> getting called from bio_free(), not bio_put().
>
> Were you the one that added that call? I know you've been working on
> that area of the code recently. Sticking it in bio_put() instead of
> bio_free() seems odd to be, and they're completely equivalent now that
> bio_free() is only called from bio_put() (save one instance I should
> probably fix).

Maybe I botched symmetry but anyways I *suspect* it probably would be
better to keep css association across bio_reset() give the current
usages of both mechanisms. css association indicates the ownership of
the bio which isn't likely to change while recycling the bio.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-05-2012, 08:13 PM
Kent Overstreet
 
Default block: Add bio_reset()

On Fri, Aug 31, 2012 at 07:23:05PM -0700, Tejun Heo wrote:
> On Tue, Aug 28, 2012 at 03:17:15PM -0700, Kent Overstreet wrote:
> > I still think they should be symmetrical, but if that's true bi_ioc and
> > bi_css need to be moved, and also bio_disassociate_task() should be
> > getting called from bio_free(), not bio_put().
> >
> > Were you the one that added that call? I know you've been working on
> > that area of the code recently. Sticking it in bio_put() instead of
> > bio_free() seems odd to be, and they're completely equivalent now that
> > bio_free() is only called from bio_put() (save one instance I should
> > probably fix).
>
> Maybe I botched symmetry but anyways I *suspect* it probably would be
> better to keep css association across bio_reset() give the current
> usages of both mechanisms. css association indicates the ownership of
> the bio which isn't likely to change while recycling the bio.

Thought about it more and while you're right that css association isn't
likely to change, it'd just be a needless difference. bio_reset() should
be as close to a bio_free()/bio_alloc() as possible, IMO.

Fixed my patches to do it right, though.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-05-2012, 08:27 PM
Kent Overstreet
 
Default block: Add bio_reset()

Reusing bios is something that's been highly frowned upon in the past,
but driver code keeps doing it anyways. If it's going to happen anyways,
we should provide a generic method.

This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
was open coding it, by doing a bio_init() and resetting bi_destructor.

v5: Add a define BIO_RESET_BITS, to be very explicit about what parts of
bio->bi_flags are saved.
v6: Further commenting verbosity, per Tejun

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
Acked-by: Tejun Heo <tj@kernel.org>
---
fs/bio.c | 14 ++++++++++++++
include/linux/bio.h | 1 +
include/linux/blk_types.h | 25 +++++++++++++++++++------
3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index b14f71a..208141f 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -262,6 +262,20 @@ void bio_init(struct bio *bio)
}
EXPORT_SYMBOL(bio_init);

+void bio_reset(struct bio *bio)
+{
+ unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
+
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, bio->bi_pool);
+
+ bio_disassociate_task(bio);
+
+ memset(bio, 0, BIO_RESET_BYTES);
+ bio->bi_flags = flags|(1 << BIO_UPTODATE);
+}
+EXPORT_SYMBOL(bio_reset);
+
/**
* bio_alloc_bioset - allocate a bio for I/O
* @gfp_mask: the GFP_ mask given to the slab allocator
diff --git a/include/linux/bio.h b/include/linux/bio.h
index a11f74b..76f6c25 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -226,6 +226,7 @@ extern void __bio_clone(struct bio *, struct bio *);
extern struct bio *bio_clone(struct bio *, gfp_t);

extern void bio_init(struct bio *);
+extern void bio_reset(struct bio *);

extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *,
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index af9dd9d..1b607c2 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -59,12 +59,6 @@ struct bio {
unsigned int bi_seg_front_size;
unsigned int bi_seg_back_size;

- unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
-
- atomic_t bi_cnt; /* pin count */
-
- struct bio_vec *bi_io_vec; /* the actual vec list */
-
bio_end_io_t *bi_end_io;

void *bi_private;
@@ -80,6 +74,16 @@ struct bio {
struct bio_integrity_payload *bi_integrity; /* data integrity */
#endif

+ /*
+ * Everything starting with bi_max_vecs will be preserved by bio_reset()
+ */
+
+ unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
+
+ atomic_t bi_cnt; /* pin count */
+
+ struct bio_vec *bi_io_vec; /* the actual vec list */
+
/* If bi_pool is non NULL, bi_destructor is not called */
struct bio_set *bi_pool;

@@ -93,6 +97,8 @@ struct bio {
struct bio_vec bi_inline_vecs[0];
};

+#define BIO_RESET_BYTES offsetof(struct bio, bi_max_vecs)
+
/*
* bio flags
*/
@@ -108,6 +114,13 @@ struct bio {
#define BIO_FS_INTEGRITY 9 /* fs owns integrity data, not block layer */
#define BIO_QUIET 10 /* Make BIO Quiet */
#define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
+
+/*
+ * Flags starting here get preserved by bio_reset() - this includes
+ * BIO_POOL_IDX()
+ */
+#define BIO_RESET_BITS 12
+
#define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))

/*
--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-06-2012, 09:32 PM
Kent Overstreet
 
Default block: Add bio_reset()

Reusing bios is something that's been highly frowned upon in the past,
but driver code keeps doing it anyways. If it's going to happen anyways,
we should provide a generic method.

This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
was open coding it, by doing a bio_init() and resetting bi_destructor.

v5: Add a define BIO_RESET_BITS, to be very explicit about what parts of
bio->bi_flags are saved.
v6: Further commenting verbosity, per Tejun

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
Acked-by: Tejun Heo <tj@kernel.org>
---
fs/bio.c | 14 ++++++++++++++
include/linux/bio.h | 1 +
include/linux/blk_types.h | 25 +++++++++++++++++++------
3 files changed, 34 insertions(+), 6 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index b14f71a..208141f 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -262,6 +262,20 @@ void bio_init(struct bio *bio)
}
EXPORT_SYMBOL(bio_init);

+void bio_reset(struct bio *bio)
+{
+ unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
+
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, bio->bi_pool);
+
+ bio_disassociate_task(bio);
+
+ memset(bio, 0, BIO_RESET_BYTES);
+ bio->bi_flags = flags|(1 << BIO_UPTODATE);
+}
+EXPORT_SYMBOL(bio_reset);
+
/**
* bio_alloc_bioset - allocate a bio for I/O
* @gfp_mask: the GFP_ mask given to the slab allocator
diff --git a/include/linux/bio.h b/include/linux/bio.h
index a11f74b..76f6c25 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -226,6 +226,7 @@ extern void __bio_clone(struct bio *, struct bio *);
extern struct bio *bio_clone(struct bio *, gfp_t);

extern void bio_init(struct bio *);
+extern void bio_reset(struct bio *);

extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *,
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index af9dd9d..1b607c2 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -59,12 +59,6 @@ struct bio {
unsigned int bi_seg_front_size;
unsigned int bi_seg_back_size;

- unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
-
- atomic_t bi_cnt; /* pin count */
-
- struct bio_vec *bi_io_vec; /* the actual vec list */
-
bio_end_io_t *bi_end_io;

void *bi_private;
@@ -80,6 +74,16 @@ struct bio {
struct bio_integrity_payload *bi_integrity; /* data integrity */
#endif

+ /*
+ * Everything starting with bi_max_vecs will be preserved by bio_reset()
+ */
+
+ unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
+
+ atomic_t bi_cnt; /* pin count */
+
+ struct bio_vec *bi_io_vec; /* the actual vec list */
+
/* If bi_pool is non NULL, bi_destructor is not called */
struct bio_set *bi_pool;

@@ -93,6 +97,8 @@ struct bio {
struct bio_vec bi_inline_vecs[0];
};

+#define BIO_RESET_BYTES offsetof(struct bio, bi_max_vecs)
+
/*
* bio flags
*/
@@ -108,6 +114,13 @@ struct bio {
#define BIO_FS_INTEGRITY 9 /* fs owns integrity data, not block layer */
#define BIO_QUIET 10 /* Make BIO Quiet */
#define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
+
+/*
+ * Flags starting here get preserved by bio_reset() - this includes
+ * BIO_POOL_IDX()
+ */
+#define BIO_RESET_BITS 12
+
#define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))

/*
--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-06-2012, 09:32 PM
Kent Overstreet
 
Default block: Add bio_reset()

Reusing bios is something that's been highly frowned upon in the past,
but driver code keeps doing it anyways. If it's going to happen anyways,
we should provide a generic method.

This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
was open coding it, by doing a bio_init() and resetting bi_destructor.

This required reordering struct bio, but the block layer is not yet
nearly fast enough for any cacheline effects to matter here.

v5: Add a define BIO_RESET_BITS, to be very explicit about what parts of
bio->bi_flags are saved.
v6: Further commenting verbosity, per Tejun
v9: Add a function comment

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
Acked-by: Tejun Heo <tj@kernel.org>
---
fs/bio.c | 24 ++++++++++++++++++++++++
1 file changed, 24 insertions(+)

diff --git a/fs/bio.c b/fs/bio.c
index 208141f..74ab3f0 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -277,6 +277,30 @@ void bio_reset(struct bio *bio)
EXPORT_SYMBOL(bio_reset);

/**
+ * bio_reset - reinitialize a bio
+ * @bio: bio to reset
+ *
+ * Description:
+ * After calling bio_reset(), @bio will be in the same state as a freshly
+ * allocated bio returned bio bio_alloc_bioset() - the only fields that are
+ * preserved are the ones that are initialized by bio_alloc_bioset(). See
+ * comment in struct bio.
+ */
+void bio_reset(struct bio *bio)
+{
+ unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
+
+ if (bio_integrity(bio))
+ bio_integrity_free(bio, bio->bi_pool);
+
+ bio_disassociate_task(bio);
+
+ memset(bio, 0, BIO_RESET_BYTES);
+ bio->bi_flags = flags|(1 << BIO_UPTODATE);
+}
+EXPORT_SYMBOL(bio_reset);
+
+/**
* bio_alloc_bioset - allocate a bio for I/O
* @gfp_mask: the GFP_ mask given to the slab allocator
* @nr_iovecs: number of iovecs to pre-allocate
--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-06-2012, 10:34 PM
Kent Overstreet
 
Default block: Add bio_reset()

Reusing bios is something that's been highly frowned upon in the past,
but driver code keeps doing it anyways. If it's going to happen anyways,
we should provide a generic method.

This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
was open coding it, by doing a bio_init() and resetting bi_destructor.

This required reordering struct bio, but the block layer is not yet
nearly fast enough for any cacheline effects to matter here.

v5: Add a define BIO_RESET_BITS, to be very explicit about what parts of
bio->bi_flags are saved.
v6: Further commenting verbosity, per Tejun
v9: Add a function comment

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
Acked-by: Tejun Heo <tj@kernel.org>
---
fs/bio.c | 24 ++++++++++++++++++++++++
include/linux/bio.h | 1 +
include/linux/blk_types.h | 25 +++++++++++++++++++------
3 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/fs/bio.c b/fs/bio.c
index b14f71a..919ee9a 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -263,6 +263,30 @@ void bio_init(struct bio *bio)
EXPORT_SYMBOL(bio_init);

/**
+ * bio_reset - reinitialize a bio
+ * @bio: bio to reset
+ *
+ * Description:
+ * After calling bio_reset(), @bio will be in the same state as a freshly
+ * allocated bio returned bio bio_alloc_bioset() - the only fields that are
+ * preserved are the ones that are initialized by bio_alloc_bioset(). See
+ * comment in struct bio.
+ */
+void bio_reset(struct bio *bio)
+{
+ unsigned long flags = bio->bi_flags & (~0UL << BIO_RESET_BITS);
+
+ if (bio_integrity(bio))
+ bio_integrity_free(bio);
+
+ bio_disassociate_task(bio);
+
+ memset(bio, 0, BIO_RESET_BYTES);
+ bio->bi_flags = flags|(1 << BIO_UPTODATE);
+}
+EXPORT_SYMBOL(bio_reset);
+
+/**
* bio_alloc_bioset - allocate a bio for I/O
* @gfp_mask: the GFP_ mask given to the slab allocator
* @nr_iovecs: number of iovecs to pre-allocate
diff --git a/include/linux/bio.h b/include/linux/bio.h
index a11f74b..76f6c25 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -226,6 +226,7 @@ extern void __bio_clone(struct bio *, struct bio *);
extern struct bio *bio_clone(struct bio *, gfp_t);

extern void bio_init(struct bio *);
+extern void bio_reset(struct bio *);

extern int bio_add_page(struct bio *, struct page *, unsigned int,unsigned int);
extern int bio_add_pc_page(struct request_queue *, struct bio *, struct page *,
diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
index af9dd9d..1b607c2 100644
--- a/include/linux/blk_types.h
+++ b/include/linux/blk_types.h
@@ -59,12 +59,6 @@ struct bio {
unsigned int bi_seg_front_size;
unsigned int bi_seg_back_size;

- unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
-
- atomic_t bi_cnt; /* pin count */
-
- struct bio_vec *bi_io_vec; /* the actual vec list */
-
bio_end_io_t *bi_end_io;

void *bi_private;
@@ -80,6 +74,16 @@ struct bio {
struct bio_integrity_payload *bi_integrity; /* data integrity */
#endif

+ /*
+ * Everything starting with bi_max_vecs will be preserved by bio_reset()
+ */
+
+ unsigned int bi_max_vecs; /* max bvl_vecs we can hold */
+
+ atomic_t bi_cnt; /* pin count */
+
+ struct bio_vec *bi_io_vec; /* the actual vec list */
+
/* If bi_pool is non NULL, bi_destructor is not called */
struct bio_set *bi_pool;

@@ -93,6 +97,8 @@ struct bio {
struct bio_vec bi_inline_vecs[0];
};

+#define BIO_RESET_BYTES offsetof(struct bio, bi_max_vecs)
+
/*
* bio flags
*/
@@ -108,6 +114,13 @@ struct bio {
#define BIO_FS_INTEGRITY 9 /* fs owns integrity data, not block layer */
#define BIO_QUIET 10 /* Make BIO Quiet */
#define BIO_MAPPED_INTEGRITY 11/* integrity metadata has been remapped */
+
+/*
+ * Flags starting here get preserved by bio_reset() - this includes
+ * BIO_POOL_IDX()
+ */
+#define BIO_RESET_BITS 12
+
#define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))

/*
--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 09-07-2012, 01:34 AM
Jens Axboe
 
Default block: Add bio_reset()

On 2012-09-06 16:34, Kent Overstreet wrote:
> Reusing bios is something that's been highly frowned upon in the past,
> but driver code keeps doing it anyways. If it's going to happen anyways,
> we should provide a generic method.
>
> This'll help with getting rid of bi_destructor - drivers/block/pktcdvd.c
> was open coding it, by doing a bio_init() and resetting bi_destructor.
>
> This required reordering struct bio, but the block layer is not yet
> nearly fast enough for any cacheline effects to matter here.

That's an odd and misplaced comment. Was just doing testing today at 5M
IOPS, and even years back we've had cache effects for O_DIRECT in higher
speed setups.

That said, we haven't done cache analysis in a long time. So moving
members around isn't necessarily a huge deal.

Lastly, this isn't a great commit message for other reasons. Anyone can
see that it moves members around. It'd be a lot better to explain _why_
it is reordering the struct.

BTW, I looked over the rest of the patches, and it looks OK to me.
However, you didn't CC me on the full series, so I'm missing bits 3 and
5.

--
Jens Axboe

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 

Thread Tools




All times are GMT. The time now is 11:28 AM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.
Copyright 2007 - 2008, www.linux-archive.org