Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   Device-mapper Development (http://www.linux-archive.org/device-mapper-development/)
-   -   block: Convert integrity to bvec_alloc_bs() (http://www.linux-archive.org/device-mapper-development/709054-block-convert-integrity-bvec_alloc_bs.html)

Vivek Goyal 10-02-2012 03:12 PM

block: Convert integrity to bvec_alloc_bs()
 
On Mon, Sep 24, 2012 at 03:34:42PM -0700, Kent Overstreet wrote:
> This adds a pointer to the bvec array to struct bio_integrity_payload,
> instead of the bvecs always being inline; then the bvecs are allocated
> with bvec_alloc_bs().

Ok, you are introducing bio_vec pointer in this patch. May be we can
do it earlier so that we take care of bio pair related issue.

>
> Changed bvec_alloc_bs() and bvec_free_bs() to take a pointer to a
> mempool instead of the bioset, so that bio integrity can use a different
> mempool for its bvecs, and thus avoid a potential deadlock.

If you are taking mempool as input, then bvec_alloc_bs() name does not
make sense, as I think the very fact "bs" in the name their suggests
that you are taking a pointer to bio set (and not mempool).

Given the fact that bio_vec pool for integrity inside bio set, why not
take additional boolean/enum argument to function bvec_alloc_bs() and
that argument can tell whehter to allocate bvec from which bvec pool.

>
> This is eventually for immutable bio vecs - immutable bvecs aren't
> useful if we still have to copy them, hence the need for the pointer.
> Less code is always nice too, though.

Can you explain a bit more about immutable bio vecs. I know there was
some discussion and I missed it. So either point me to right mail thread
or just explain a bit more what are immutable bio vecs, how we are
planning to use these.

Is it about during split we don't want to copy bio vecs and that's why we
need a pointer so that newly created bio/bip can point to parent's bio_vec?

Thanks
Vivek

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

Vivek Goyal 10-02-2012 03:37 PM

block: Convert integrity to bvec_alloc_bs()
 
On Mon, Sep 24, 2012 at 03:34:42PM -0700, Kent Overstreet wrote:

[..]
> /**
> * bio_integrity_alloc - Allocate integrity payload and attach it to bio
> * @bio: bio to attach integrity metadata to
> @@ -84,37 +47,39 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
> unsigned int nr_vecs)
> {
> struct bio_integrity_payload *bip;
> - unsigned int idx = vecs_to_idx(nr_vecs);
> struct bio_set *bs = bio->bi_pool;
> + unsigned long idx = BIO_POOL_NONE;
> + unsigned inline_vecs;
> +
> + if (!bs) {
> + bip = kmalloc(sizeof(struct bio_integrity_payload) +
> + sizeof(struct bio_vec) * nr_vecs, gfp_mask);
> + inline_vecs = nr_vecs;
> + } else {
> + bip = mempool_alloc(bs->bio_integrity_pool, gfp_mask);
> + inline_vecs = BIP_INLINE_VECS;
> + }
>
> - if (!bs)
> - bs = fs_bio_set;

Ok, this is change of behavior. Previously we will fall back to fs_bio_set
and now you do kmalloc. This change looks to be independent of bip_vec
pointer. Can you please break it out in a separate patch and also explain
that how does this change help.

Thanks
Vivek

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

Kent Overstreet 10-02-2012 08:52 PM

block: Convert integrity to bvec_alloc_bs()
 
On Tue, Oct 02, 2012 at 11:12:02AM -0400, Vivek Goyal wrote:
> On Mon, Sep 24, 2012 at 03:34:42PM -0700, Kent Overstreet wrote:
> > This adds a pointer to the bvec array to struct bio_integrity_payload,
> > instead of the bvecs always being inline; then the bvecs are allocated
> > with bvec_alloc_bs().
>
> Ok, you are introducing bio_vec pointer in this patch. May be we can
> do it earlier so that we take care of bio pair related issue.

I was just trying to make the bugfix patch small, since people
complained about too much stuff being done in one patch.

> > Changed bvec_alloc_bs() and bvec_free_bs() to take a pointer to a
> > mempool instead of the bioset, so that bio integrity can use a different
> > mempool for its bvecs, and thus avoid a potential deadlock.
>
> If you are taking mempool as input, then bvec_alloc_bs() name does not
> make sense, as I think the very fact "bs" in the name their suggests
> that you are taking a pointer to bio set (and not mempool).
>
> Given the fact that bio_vec pool for integrity inside bio set, why not
> take additional boolean/enum argument to function bvec_alloc_bs() and
> that argument can tell whehter to allocate bvec from which bvec pool.

Eww, boolean arguments are always bad.

You are right about bvec_alloc_bs() being misnamed now... honestly it
doesn't bother me, but it should probably just be renamed to
bvec_alloc().

Much prefer the mempool arg to a boolean, though.

> > This is eventually for immutable bio vecs - immutable bvecs aren't
> > useful if we still have to copy them, hence the need for the pointer.
> > Less code is always nice too, though.
>
> Can you explain a bit more about immutable bio vecs. I know there was
> some discussion and I missed it. So either point me to right mail thread
> or just explain a bit more what are immutable bio vecs, how we are
> planning to use these.

http://article.gmane.org/gmane.linux.kernel.bcache.devel/890

That's my writeup - Tejun and I have been talking about this for ages,
and then Martin Petersen independently brought it up recently and that
was when I decided to just dive in and do it - but they'd probably have
their own ideas about what it's good for. (Tejun wants to reduce the
number of different scatter/gather list implementations).

> Is it about during split we don't want to copy bio vecs and that's why we
> need a pointer so that newly created bio/bip can point to parent's bio_vec?

That's one reason. It enables efficient arbitrary bio splitting, which
enables a whole host of cleanups and simplifications (I've deleted ~1500
lines of code in my tree).

Also, the fact that bv_len and bv_offset are modified is a pain; the
owner of the bio can't use them after the bio has completed, if it
needed to know what memory the bio pointed to it's got to store that
somewhere else. This fixes that.

It's a real pain for error handling in stacking block drivers - if they
want to be able to retry the bio, they have to clone not just the bio
itself but the entire bvec.

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

Kent Overstreet 10-02-2012 09:00 PM

block: Convert integrity to bvec_alloc_bs()
 
On Tue, Oct 02, 2012 at 11:37:37AM -0400, Vivek Goyal wrote:
> On Mon, Sep 24, 2012 at 03:34:42PM -0700, Kent Overstreet wrote:
>
> [..]
> > /**
> > * bio_integrity_alloc - Allocate integrity payload and attach it to bio
> > * @bio: bio to attach integrity metadata to
> > @@ -84,37 +47,39 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
> > unsigned int nr_vecs)
> > {
> > struct bio_integrity_payload *bip;
> > - unsigned int idx = vecs_to_idx(nr_vecs);
> > struct bio_set *bs = bio->bi_pool;
> > + unsigned long idx = BIO_POOL_NONE;
> > + unsigned inline_vecs;
> > +
> > + if (!bs) {
> > + bip = kmalloc(sizeof(struct bio_integrity_payload) +
> > + sizeof(struct bio_vec) * nr_vecs, gfp_mask);
> > + inline_vecs = nr_vecs;
> > + } else {
> > + bip = mempool_alloc(bs->bio_integrity_pool, gfp_mask);
> > + inline_vecs = BIP_INLINE_VECS;
> > + }
> >
> > - if (!bs)
> > - bs = fs_bio_set;
>
> Ok, this is change of behavior. Previously we will fall back to fs_bio_set
> and now you do kmalloc. This change looks to be independent of bip_vec
> pointer. Can you please break it out in a separate patch and also explain
> that how does this change help.

I'm not sure it's worth breaking out into a separate patch, but I
definitely should've mentioned it in the patch description.

It just didn't make sense to be using fs_bio_set if a bio_set wasn't
specified before - if a bio set wasn't specified we're still using
kmalloc for the bio_integrity_payload, so we're not protected from
memory allocation failures and it doesn't buy us anything.

All it does is introduce the possibility of deadlock, if we weren't
supposed to be using fs_bio_set for whatever reason.

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

Vivek Goyal 10-02-2012 10:02 PM

block: Convert integrity to bvec_alloc_bs()
 
On Tue, Oct 02, 2012 at 02:00:06PM -0700, Kent Overstreet wrote:
> On Tue, Oct 02, 2012 at 11:37:37AM -0400, Vivek Goyal wrote:
> > On Mon, Sep 24, 2012 at 03:34:42PM -0700, Kent Overstreet wrote:
> >
> > [..]
> > > /**
> > > * bio_integrity_alloc - Allocate integrity payload and attach it to bio
> > > * @bio: bio to attach integrity metadata to
> > > @@ -84,37 +47,39 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio,
> > > unsigned int nr_vecs)
> > > {
> > > struct bio_integrity_payload *bip;
> > > - unsigned int idx = vecs_to_idx(nr_vecs);
> > > struct bio_set *bs = bio->bi_pool;
> > > + unsigned long idx = BIO_POOL_NONE;
> > > + unsigned inline_vecs;
> > > +
> > > + if (!bs) {
> > > + bip = kmalloc(sizeof(struct bio_integrity_payload) +
> > > + sizeof(struct bio_vec) * nr_vecs, gfp_mask);
> > > + inline_vecs = nr_vecs;
> > > + } else {
> > > + bip = mempool_alloc(bs->bio_integrity_pool, gfp_mask);
> > > + inline_vecs = BIP_INLINE_VECS;
> > > + }
> > >
> > > - if (!bs)
> > > - bs = fs_bio_set;
> >
> > Ok, this is change of behavior. Previously we will fall back to fs_bio_set
> > and now you do kmalloc. This change looks to be independent of bip_vec
> > pointer. Can you please break it out in a separate patch and also explain
> > that how does this change help.
>
> I'm not sure it's worth breaking out into a separate patch, but I
> definitely should've mentioned it in the patch description.

It helps a lot with reviewing the patches.
>
> It just didn't make sense to be using fs_bio_set if a bio_set wasn't
> specified before - if a bio set wasn't specified we're still using
> kmalloc for the bio_integrity_payload, so we're not protected from
> memory allocation failures and it doesn't buy us anything.
>
> All it does is introduce the possibility of deadlock, if we weren't
> supposed to be using fs_bio_set for whatever reason.

I think more people will be willing to review patches if you break
them down in to small patches with proper explanation. This particular
change is orthogonal to allocating integrity vecs from bioset. So
really think it does make sense to keep this change in a separate
patch with proper changelog.

Thanks
Vivek

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

Vivek Goyal 10-02-2012 10:05 PM

block: Convert integrity to bvec_alloc_bs()
 
On Tue, Oct 02, 2012 at 01:52:50PM -0700, Kent Overstreet wrote:
> On Tue, Oct 02, 2012 at 11:12:02AM -0400, Vivek Goyal wrote:
> > On Mon, Sep 24, 2012 at 03:34:42PM -0700, Kent Overstreet wrote:
> > > This adds a pointer to the bvec array to struct bio_integrity_payload,
> > > instead of the bvecs always being inline; then the bvecs are allocated
> > > with bvec_alloc_bs().
> >
> > Ok, you are introducing bio_vec pointer in this patch. May be we can
> > do it earlier so that we take care of bio pair related issue.
>
> I was just trying to make the bugfix patch small, since people
> complained about too much stuff being done in one patch.

Can't we introduce the pointer while we retain bip_slabs as it is. Then
this patch will be small. I think this patch is big only because you
are trying to allocate integrity vecs from bio_set out of line.

Thanks
Vivek

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

Kent Overstreet 10-02-2012 10:17 PM

block: Convert integrity to bvec_alloc_bs()
 
On Tue, Oct 02, 2012 at 06:05:53PM -0400, Vivek Goyal wrote:
> On Tue, Oct 02, 2012 at 01:52:50PM -0700, Kent Overstreet wrote:
> > On Tue, Oct 02, 2012 at 11:12:02AM -0400, Vivek Goyal wrote:
> > > On Mon, Sep 24, 2012 at 03:34:42PM -0700, Kent Overstreet wrote:
> > > > This adds a pointer to the bvec array to struct bio_integrity_payload,
> > > > instead of the bvecs always being inline; then the bvecs are allocated
> > > > with bvec_alloc_bs().
> > >
> > > Ok, you are introducing bio_vec pointer in this patch. May be we can
> > > do it earlier so that we take care of bio pair related issue.
> >
> > I was just trying to make the bugfix patch small, since people
> > complained about too much stuff being done in one patch.
>
> Can't we introduce the pointer while we retain bip_slabs as it is. Then
> this patch will be small. I think this patch is big only because you
> are trying to allocate integrity vecs from bio_set out of line.

Ok - yeah, that makes sense to break out. I think I'll just make the
integrity stuff a separate patch series, it's going to be like 4 patches
now.

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


All times are GMT. The time now is 10:11 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.