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 05-18-2012, 04:30 PM
Tejun Heo
 
Default block: Add an explicit bio flag for bios that own their bvec

On Thu, May 17, 2012 at 10:59:56PM -0400, koverstreet@google.com wrote:
> From: Kent Overstreet <koverstreet@google.com>
>
> This is for the new bio splitting code. When we split a bio, if the
> split occured on a bvec boundry we reuse the bvec for the new bio. But
> that means bio_free() can't free it, hence the explicit flag.
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> ---
> fs/bio.c | 3 ++-
> include/linux/blk_types.h | 1 +
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/fs/bio.c b/fs/bio.c
> index ecc9088..3332800 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -234,7 +234,7 @@ void bio_free(struct bio *bio, struct bio_set *bs)
> {
> void *p;
>
> - if (bio_has_allocated_vec(bio))
> + if (bio_flagged(bio, BIO_HAS_VEC))
> bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));

We can remove bio_has_allocated_vec()?

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-18-2012, 05:07 PM
Boaz Harrosh
 
Default block: Add an explicit bio flag for bios that own their bvec

On 05/18/2012 05:59 AM, koverstreet@google.com wrote:

> From: Kent Overstreet <koverstreet@google.com>
>
> This is for the new bio splitting code. When we split a bio, if the
> split occured on a bvec boundry we reuse the bvec for the new bio. But
> that means bio_free() can't free it, hence the explicit flag.
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> ---
> fs/bio.c | 3 ++-
> include/linux/blk_types.h | 1 +
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/fs/bio.c b/fs/bio.c
> index ecc9088..3332800 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -234,7 +234,7 @@ void bio_free(struct bio *bio, struct bio_set *bs)
> {
> void *p;
>
> - if (bio_has_allocated_vec(bio))
> + if (bio_flagged(bio, BIO_HAS_VEC))
> bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
>
> if (bio_integrity(bio))
> @@ -305,6 +305,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
> goto err_free;
>
> nr_iovecs = bvec_nr_vecs(idx);
> + bio->bi_flags |= 1 << BIO_HAS_VEC;
> }
> out_set:
> bio->bi_flags |= idx << BIO_POOL_OFFSET;
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index bd9a610..10fca21 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -101,6 +101,7 @@ 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 */
> +#define BIO_HAS_VEC 12 /* bio_free() should free bvec */


Just an English nit
BIO with out any VECs is when bi_max_vecs is zero. Id prefer a name that
denotes allocation or ownership.
BIO_OWN_VEC or BIO_ALLOC_VEC or BIO_DELETE_VEC

And one technical problem.

I would prefer to negate this bit. Zero is the default, set on
the exception. Safer and also more logical.

BIO_NO_DELETE_VEC

> #define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))
>
> /*



Thanks
Boaz

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-18-2012, 09:49 PM
Kent Overstreet
 
Default block: Add an explicit bio flag for bios that own their bvec

On Fri, May 18, 2012 at 09:30:18AM -0700, Tejun Heo wrote:
> On Thu, May 17, 2012 at 10:59:56PM -0400, koverstreet@google.com wrote:
> > From: Kent Overstreet <koverstreet@google.com>
> >
> > This is for the new bio splitting code. When we split a bio, if the
> > split occured on a bvec boundry we reuse the bvec for the new bio. But
> > that means bio_free() can't free it, hence the explicit flag.
> >
> > Signed-off-by: Kent Overstreet <koverstreet@google.com>
> > ---
> > fs/bio.c | 3 ++-
> > include/linux/blk_types.h | 1 +
> > 2 files changed, 3 insertions(+), 1 deletions(-)
> >
> > diff --git a/fs/bio.c b/fs/bio.c
> > index ecc9088..3332800 100644
> > --- a/fs/bio.c
> > +++ b/fs/bio.c
> > @@ -234,7 +234,7 @@ void bio_free(struct bio *bio, struct bio_set *bs)
> > {
> > void *p;
> >
> > - if (bio_has_allocated_vec(bio))
> > + if (bio_flagged(bio, BIO_HAS_VEC))
> > bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
>
> We can remove bio_has_allocated_vec()?

Yep! Adding that to the patch.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-24-2012, 04:57 PM
Boaz Harrosh
 
Default block: Add an explicit bio flag for bios that own their bvec

On 05/24/2012 03:02 AM, Kent Overstreet wrote:

> This is for the new bio splitting code. When we split a bio, if the
> split occured on a bvec boundry we reuse the bvec for the new bio. But
> that means bio_free() can't free it, hence the explicit flag.
>




How do you insure that the original bio which owns the
bvec is not freed before the split-out bio.

Perhaps calling code needs to make sure by taking an extra
ref on the original bio, or something. If so a big fat comment
at bio_split is do.

And I understand you did not like my suggestion of negating
the meaning of the flag, so the default is zero?
Please say why?

Thanks
Boaz

> Signed-off-by: Kent Overstreet <koverstreet@google.com>
> Change-Id: I040f6b501088e882a9f013d6b6e730ff04e9c1da
> ---
> fs/bio.c | 3 ++-
> include/linux/bio.h | 5 -----
> include/linux/blk_types.h | 1 +
> 3 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/fs/bio.c b/fs/bio.c
> index fc4a168..24b1d1c 100644
> --- a/fs/bio.c
> +++ b/fs/bio.c
> @@ -241,7 +241,7 @@ void bio_free(struct bio *bio, struct bio_set *bs)
> return;
> }
>
> - if (bio_has_allocated_vec(bio))
> + if (bio_flagged(bio, BIO_HAS_VEC))
> bvec_free_bs(bs, bio->bi_io_vec, BIO_POOL_IDX(bio));
>
> if (bio_integrity(bio))
> @@ -312,6 +312,7 @@ struct bio *bio_alloc_bioset(gfp_t gfp_mask, int nr_iovecs, struct bio_set *bs)
> goto err_free;
>
> nr_iovecs = bvec_nr_vecs(idx);
> + bio->bi_flags |= 1 << BIO_HAS_VEC;
> }
> out_set:
> bio->bi_flags |= idx << BIO_POOL_OFFSET;
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 35f7c4d..8dd8cae 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -84,11 +84,6 @@ static inline void *bio_data(struct bio *bio)
> return NULL;
> }
>
> -static inline int bio_has_allocated_vec(struct bio *bio)
> -{
> - return bio->bi_io_vec && bio->bi_io_vec != bio->bi_inline_vecs;
> -}
> -
> /*
> * will die
> */
> diff --git a/include/linux/blk_types.h b/include/linux/blk_types.h
> index b6ddbf1..ab2252d 100644
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -101,6 +101,7 @@ 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 */
> +#define BIO_HAS_VEC 12 /* bio_free() should free bvec */
> #define bio_flagged(bio, flag) ((bio)->bi_flags & (1 << (flag)))
>
> /*




--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-24-2012, 09:31 PM
Kent Overstreet
 
Default block: Add an explicit bio flag for bios that own their bvec

On Thu, May 24, 2012 at 07:57:34PM +0300, Boaz Harrosh wrote:
> How do you insure that the original bio which owns the
> bvec is not freed before the split-out bio.
>
> Perhaps calling code needs to make sure by taking an extra
> ref on the original bio, or something. If so a big fat comment
> at bio_split is do.

Yeah, just added that.

>
> And I understand you did not like my suggestion of negating
> the meaning of the flag, so the default is zero?
> Please say why?

I liked it at first, but I think I prefer having the flag be set
if bio_free() must take some action; i.e. you set the flag when you
allocate bi_io_vec. Also, I think bio_alloc_bioset() getting
reimplemented is less likely than people open coding bio splitting or
something that shares bi_io_vec in the future, so it's slightlry less
likely to be used wrong this way.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-25-2012, 04:49 PM
Vivek Goyal
 
Default block: Add an explicit bio flag for bios that own their bvec

On Thu, May 24, 2012 at 02:31:58PM -0700, Kent Overstreet wrote:
> On Thu, May 24, 2012 at 07:57:34PM +0300, Boaz Harrosh wrote:
> > How do you insure that the original bio which owns the
> > bvec is not freed before the split-out bio.
> >
> > Perhaps calling code needs to make sure by taking an extra
> > ref on the original bio, or something. If so a big fat comment
> > at bio_split is do.
>
> Yeah, just added that.
>
> >
> > And I understand you did not like my suggestion of negating
> > the meaning of the flag, so the default is zero?
> > Please say why?
>
> I liked it at first, but I think I prefer having the flag be set
> if bio_free() must take some action; i.e. you set the flag when you
> allocate bi_io_vec. Also, I think bio_alloc_bioset() getting
> reimplemented is less likely than people open coding bio splitting or
> something that shares bi_io_vec in the future, so it's slightlry less
> likely to be used wrong this way.

Even if you keep it as it is, I thought BIO_OWNS_BVEC probably communicates
the idea better than BIO_HAS_BVEC.

Thanks
Vivek

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-25-2012, 08:01 PM
Kent Overstreet
 
Default block: Add an explicit bio flag for bios that own their bvec

On Fri, May 25, 2012 at 12:49:14PM -0400, Vivek Goyal wrote:
> Even if you keep it as it is, I thought BIO_OWNS_BVEC probably communicates
> the idea better than BIO_HAS_BVEC.

Yeah, I'll make that change.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 05-28-2012, 01:52 AM
Tejun Heo
 
Default block: Add an explicit bio flag for bios that own their bvec

On Fri, May 25, 2012 at 01:25:29PM -0700, Kent Overstreet wrote:
> This is for the new bio splitting code. When we split a bio, if the
> split occured on a bvec boundry we reuse the bvec for the new bio. But
> that means bio_free() can't free it, hence the explicit flag.

I keep having the same complaints. The explanation isn't detailed
enough. So, the necessity for this patch arises from the fact that
the current bio_split doesn't use the usual bio allocation path to
create split bio - it just supports fixed bvec allocation via bio_pair
allocation which is allowable because of the severe restrictions the
current bio_split() - but the new bio_split() wants to do it in
general manner and thus wants the usual bvec allocation.

Why do I (or anyone for that matter) have to go forward in the patch
series to reconstruct the rationale? Why doesn't the patch
description already explain this? Why isn't this still fixed when
lack of proper patch description has already been pointed out multiple
times?

I'm gonna stop here on this series.

* *Please* spend more effort on patch description and understanding
and applying the reviews. If you don't like the opinions expressed
in reviews, please argue. If reviews get ignored, why would anyone
review your patches at all?

* It would probably be better to split the series so that more
experimental / constroversial stuff is in separate patch series.

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-08-2012, 10:28 PM
Tejun Heo
 
Default block: Add an explicit bio flag for bios that own their bvec

On Mon, Aug 06, 2012 at 03:08:35PM -0700, Kent Overstreet wrote:
> This is for the new bio splitting code. When we split a bio, if the
> split occured on a bvec boundry we reuse the bvec for the new bio. But
> that means bio_free() can't free it, hence the explicit flag.
>
> Signed-off-by: Kent Overstreet <koverstreet@google.com>

Sans how the flag is preserved,

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

--
tejun

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-22-2012, 05:43 PM
Adrian Bunk
 
Default block: Add an explicit bio flag for bios that own their bvec

On Wed, Aug 22, 2012 at 10:04:05AM -0700, Kent Overstreet wrote:
>...
> --- a/include/linux/blk_types.h
> +++ b/include/linux/blk_types.h
> @@ -117,6 +117,7 @@ struct bio {
> * BIO_POOL_IDX()
> */
> #define BIO_RESET_BITS 12
> +#define BIO_OWNS_VEC 12 /* bio_free() should free bvec */
>...

This doesn't look right.

cu
Adrian

--

"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed

--
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 05:54 AM.

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