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-09-2012, 07:33 AM
Kent Overstreet
 
Default block: Introduce new bio_split()

On Thu, Aug 09, 2012 at 12:22:17AM -0700, Tejun Heo wrote:
> On Wed, Aug 08, 2012 at 06:39:23PM -0700, Kent Overstreet wrote:
> > On Wed, Aug 08, 2012 at 04:05:32PM -0700, Tejun Heo wrote:
> > > One more thing.
> > >
> > > On Mon, Aug 06, 2012 at 03:08:37PM -0700, Kent Overstreet wrote:
> > > > + if (bio_integrity(bio)) {
> > > > + bio_integrity_clone(ret, bio, gfp, bs);
> > > > + bio_integrity_trim(ret, 0, bio_sectors(ret));
> > > > + bio_integrity_trim(bio, bio_sectors(ret), bio_sectors(bio));
> > >
> > > Is this equivalent to bio_integrity_split() performance-wise?
> >
> > Strictly speaking, no. But it has the advantage of being drastically
> > simpler - and the only one only worked for single page bios so I
> > would've had to come up with something new from scratch, and as
> > confusing as the integrity stuff is I wouldn't trust the result.
>
> There's already bio_integrity_split() and you're actively dropping it.

Because it only works for single page bios, AFAICT. I'd have to start
from scratch.

> > I'm skeptical that it's going to matter in practice given how much
> > iteration is done elsewhere in the course of processing a bio and given
> > that this stuff isn't used with high end SSDs...
>
> If you think the active dropping is justified, please let the change
> and justification clearly stated. You're burying the active change in
> two separate patches without even mentioning it or cc'ing people who
> care about bio-integrity (Martin K. Petersen).

Not intentionally, he isn't in MAINTAINERS so get_maintainers.pl missed
it and it slipped by while I was looking for people to CC. Added him.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-09-2012, 05:32 PM
Tejun Heo
 
Default block: Introduce new bio_split()

Hello,

On Thu, Aug 09, 2012 at 03:33:34AM -0400, Kent Overstreet wrote:
> > If you think the active dropping is justified, please let the change
> > and justification clearly stated. You're burying the active change in
> > two separate patches without even mentioning it or cc'ing people who
> > care about bio-integrity (Martin K. Petersen).
>
> Not intentionally, he isn't in MAINTAINERS so get_maintainers.pl missed
> it and it slipped by while I was looking for people to CC. Added him.

git-log is your friend. For one-off patches, doing it this way might
be okay. Higher layer maintainer would be able to redirect it but if
you intend to change block layer APIs significantly as you try to do
in this patch series, you need to be *way* more diligent than you
currently are. At least I feel risky about acking patches in this
series.

* Significant change is buried without explicitly mentioning it or
discussing its implications.

* The patchset makes block layer API changes which impact multiple
stacking and low level drivers which are not particularly known for
simplicity and robustness, but there's no mention of how the patches
are tested and/or why the patches would be safe (e.g. reviewed all
the users and tested certain code paths and am fairly sure all the
changes should be safe because xxx sort of deal). When asked about
testing, not much seems to have been done.

* Responses and iterations across patch postings aren't responsive or
reliable, making it worrisome what will happen when things go south
after this hits mainline.

You're asking reviewers and maintainers to take a lot more risks than
they usually have to, which isn't a good way to make forward progress.

Thanks.

--
tejun

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

On Wed, Aug 08, 2012 at 03:58:39PM -0700, Tejun Heo wrote:
> On Mon, Aug 06, 2012 at 03:08:37PM -0700, Kent Overstreet wrote:
> > + *
> > + * BIG FAT WARNING:
> > + *
> > + * If you're calling this from under generic_make_request() (i.e.
> > + * current->bio_list != NULL), you should mask out __GFP_WAIT and punt to
> > + * workqueue if the allocation fails. Otherwise, your code will probably
> > + * deadlock.
>
> If the condition is detectable, WARN_ON_ONCE() please.

I know I said I liked this idea, but I changed my mind.

Sticking a WARN_ON_ONCE() there is saying that passing __GFP_WAIT from
under generic_make_request() is always wrong - it might as well be a
BUG_ON() except warn is better for the user.

If that's true, then an assertion is completely wrong because we can
just do the right thing instead - mask out __GFP_WAIT if
current->bio_list != NULL and document that it can fail in that
situation.

Which is what my original code did.

The alternative is accepting that there are situations where it is
technically possible to pass __GFP_WAIT from under
generic_make_request() without deadlocking and allow it, but my position
is still that that is far too subtle to expect that it'll be gotten
right (especially considering the ways that the code is wrong today
wrt deadlocks).

But honestly this is turning into bikeshedding. The current bio
splitting and merge_bvec_fn stuff is crap, and there are worse potential
deadlocks/bugs in the existing code than what we're arguing over here.

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-13-2012, 10:05 PM
Tejun Heo
 
Default block: Introduce new bio_split()

Hello,

On Mon, Aug 13, 2012 at 02:55:11PM -0700, Kent Overstreet wrote:
> > If the condition is detectable, WARN_ON_ONCE() please.
>
> I know I said I liked this idea, but I changed my mind.
>
> Sticking a WARN_ON_ONCE() there is saying that passing __GFP_WAIT from
> under generic_make_request() is always wrong - it might as well be a
> BUG_ON() except warn is better for the user.

WARN_ON_ONCE(), as opposed to BUG_ON(), doesn't mean that something
isn't always wrong. It's just better to always use WARN variant if
the system doesn't *need* to be put down immediately to avoid, say,
massive data corruption - limping machine is simply better than dead
one.

> If that's true, then an assertion is completely wrong because we can
> just do the right thing instead - mask out __GFP_WAIT if
> current->bio_list != NULL and document that it can fail in that
> situation.

That's worse because it confuses the reader. Taking something which
is as well-known as __GFP_WAIT and then silently ignoring it isn't a
good idea. Please just add a WARN_ON.

Thanks.

--
tejun

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

On Thu, Aug 09, 2012 at 10:32:17AM -0700, Tejun Heo wrote:
> Hello,
>
> On Thu, Aug 09, 2012 at 03:33:34AM -0400, Kent Overstreet wrote:
> > > If you think the active dropping is justified, please let the change
> > > and justification clearly stated. You're burying the active change in
> > > two separate patches without even mentioning it or cc'ing people who
> > > care about bio-integrity (Martin K. Petersen).
> >
> > Not intentionally, he isn't in MAINTAINERS so get_maintainers.pl missed
> > it and it slipped by while I was looking for people to CC. Added him.
>
> git-log is your friend. For one-off patches, doing it this way might
> be okay. Higher layer maintainer would be able to redirect it but if
> you intend to change block layer APIs significantly as you try to do
> in this patch series, you need to be *way* more diligent than you
> currently are. At least I feel risky about acking patches in this
> series.

Wasn't an excuse, just an explanation.

> * Significant change is buried without explicitly mentioning it or
> discussing its implications.

You are entitled to your opinion, but it honestly didn't seem that
significant to me considering there was no code for splitting multi page
bio integrity stuff before.

> * The patchset makes block layer API changes which impact multiple
> stacking and low level drivers which are not particularly known for
> simplicity and robustness, but there's no mention of how the patches
> are tested and/or why the patches would be safe (e.g. reviewed all
> the users and tested certain code paths and am fairly sure all the
> changes should be safe because xxx sort of deal). When asked about
> testing, not much seems to have been done.

You picked a particularly obscure codepath. I have personally tested all
of this code with both dm and md, and I spent quite a bit of time going
over the dm code in particular to verify as best I could that the
bio_clone() changes were safe.

I should've said more before how the patch series as a whole was tested,
but you hadn't asked either.

> * Responses and iterations across patch postings aren't responsive or
> reliable, making it worrisome what will happen when things go south
> after this hits mainline.
>
> You're asking reviewers and maintainers to take a lot more risks than
> they usually have to, which isn't a good way to make forward progress.

This patch series does touch a lot, and I'm certainly very new at
getting stuff into upstream. But I'm doing my best not to miss stuff,
and I've been asking you for advice and working on my workflow. And a
fair amount of the stuff in this patch series has been your ideas (it
was originally much more minimal).

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-22-2012, 05:04 PM
Kent Overstreet
 
Default block: Introduce new bio_split()

The new bio_split() can split arbitrary bios - it's not restricted to
single page bios, like the old bio_split() (previously renamed to
bio_pair_split()). It also has different semantics - it doesn't allocate
a struct bio_pair, leaving it up to the caller to handle completions.

We have to take that BUG_ON() out of bio_integrity_trim() because this
bio_split() needs to use it, and there's no reason it has to be used on
bios marked as cloned; BIO_CLONED doesn't seem to have clearly
documented semantics anyways.

v5: Take out current->bio_list check and make it the caller's
responsibility, per Boaz
v6: Rename @ret to @split, change to not return original bio or copy
bi_private/bi_end_io, per Tejun

Signed-off-by: Kent Overstreet <koverstreet@google.com>
CC: Jens Axboe <axboe@kernel.dk>
CC: Martin K. Petersen <martin.petersen@oracle.com>
---
fs/bio-integrity.c | 1 -
fs/bio.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++++ +++
include/linux/bio.h | 13 ++++++++
3 files changed, 108 insertions(+), 1 deletion(-)

diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index e85c04b..35ee3d4 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -672,7 +672,6 @@ void bio_integrity_trim(struct bio *bio, unsigned int offset,

BUG_ON(bip == NULL);
BUG_ON(bi == NULL);
- BUG_ON(!bio_flagged(bio, BIO_CLONED));

nr_sectors = bio_integrity_hw_sectors(bi, sectors);
bip->bip_sector = bip->bip_sector + offset;
diff --git a/fs/bio.c b/fs/bio.c
index 2f2cf19..c079006 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -1541,6 +1541,101 @@ struct bio_pair *bio_pair_split(struct bio *bi, int first_sectors)
EXPORT_SYMBOL(bio_pair_split);

/**
+ * bio_split - split a bio
+ * @bio: bio to split
+ * @sectors: number of sectors to split from the front of @bio
+ * @gfp: gfp mask
+ * @bs: bio set to allocate from
+ *
+ * Allocates and returns a new bio which represents @sectors from the start of
+ * @bio, and updates @bio to represent the remaining sectors.
+ *
+ * If bio_sectors(@bio) was less than or equal to @sectors, returns @bio
+ * unchanged.
+ *
+ * The newly allocated bio will point to @bio's bi_io_vec, if the split was on a
+ * bvec boundry; it is the caller's responsibility to ensure that @bio is not
+ * freed before the split.
+ */
+struct bio *bio_split(struct bio *bio, int sectors,
+ gfp_t gfp, struct bio_set *bs)
+{
+ unsigned idx, vcnt = 0, nbytes = sectors << 9;
+ struct bio_vec *bv;
+ struct bio *split = NULL;
+
+ BUG_ON(sectors <= 0);
+ BUG_ON(sectors >= bio_sectors(bio));
+
+ trace_block_split(bdev_get_queue(bio->bi_bdev), bio,
+ bio->bi_sector + sectors);
+
+ if (bio->bi_rw & REQ_DISCARD) {
+ split = bio_alloc_bioset(gfp, 1, bs);
+
+ split->bi_io_vec = bio_iovec(bio);
+ idx = 0;
+ goto out;
+ }
+
+ bio_for_each_segment(bv, bio, idx) {
+ vcnt = idx - bio->bi_idx;
+
+ /*
+ * The split might occur on a bvec boundry (nbytes == 0) - in
+ * that case we can reuse the bvec from the old bio.
+ *
+ * Or, if the split isn't aligned, we'll have to allocate a new
+ * bvec and patch up the two copies of the bvec we split in.
+ */
+
+ if (!nbytes) {
+ split = bio_alloc_bioset(gfp, 0, bs);
+ if (!split)
+ return NULL;
+
+ split->bi_io_vec = bio_iovec(bio);
+ split->bi_flags |= 1 << BIO_CLONED;
+ break;
+ } else if (nbytes < bv->bv_len) {
+ split = bio_alloc_bioset(gfp, ++vcnt, bs);
+ if (!split)
+ return NULL;
+
+ memcpy(split->bi_io_vec, bio_iovec(bio),
+ sizeof(struct bio_vec) * vcnt);
+
+ split->bi_io_vec[vcnt - 1].bv_len = nbytes;
+ bv->bv_offset += nbytes;
+ bv->bv_len -= nbytes;
+ break;
+ }
+
+ nbytes -= bv->bv_len;
+ }
+out:
+ split->bi_bdev = bio->bi_bdev;
+ split->bi_sector = bio->bi_sector;
+ split->bi_size = sectors << 9;
+ split->bi_rw = bio->bi_rw;
+ split->bi_vcnt = vcnt;
+ split->bi_max_vecs = vcnt;
+
+ bio->bi_sector += sectors;
+ bio->bi_size -= sectors << 9;
+ bio->bi_idx = idx;
+
+ if (bio_integrity(bio)) {
+ bio_integrity_clone(split, bio, gfp, bs);
+ bio_integrity_trim(split, 0, bio_sectors(split));
+ bio_integrity_trim(bio, bio_sectors(split), bio_sectors(bio));
+ }
+
+ return split;
+}
+EXPORT_SYMBOL_GPL(bio_split);
+
+/**
* bio_sector_offset - Find hardware sector offset in bio
* @bio: bio to inspect
* @index: bio_vec index
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 13f4188..1c3bb47 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -201,6 +201,19 @@ struct bio_pair {
atomic_t cnt;
int error;
};
+
+extern struct bio *bio_split(struct bio *bio, int sectors,
+ gfp_t gfp, struct bio_set *bs);
+
+static inline struct bio *bio_next_split(struct bio *bio, int sectors,
+ gfp_t gfp, struct bio_set *bs)
+{
+ if (sectors >= bio_sectors(bio))
+ return bio;
+
+ return bio_split(bio, sectors, gfp, bs);
+}
+
extern struct bio_pair *bio_pair_split(struct bio *bi, int first_sectors);
extern void bio_pair_release(struct bio_pair *dbio);

--
1.7.12

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel
 
Old 08-22-2012, 08:46 PM
Tejun Heo
 
Default block: Introduce new bio_split()

Hello, Kent.

On Wed, Aug 22, 2012 at 10:04:07AM -0700, Kent Overstreet wrote:
> @@ -672,7 +672,6 @@ void bio_integrity_trim(struct bio *bio, unsigned int offset,
>
> BUG_ON(bip == NULL);
> BUG_ON(bi == NULL);
> - BUG_ON(!bio_flagged(bio, BIO_CLONED));
>
> nr_sectors = bio_integrity_hw_sectors(bi, sectors);
> bip->bip_sector = bip->bip_sector + offset;

It depends on what BIO_CLONED is supposed to do but maybe the right
thing to do is clearing BIO_CLONED on split bio? The split bio isn't
a clone after all.

> /**
> + * bio_split - split a bio
> + * @bio: bio to split
> + * @sectors: number of sectors to split from the front of @bio
> + * @gfp: gfp mask
> + * @bs: bio set to allocate from
> + *
> + * Allocates and returns a new bio which represents @sectors from the start of
> + * @bio, and updates @bio to represent the remaining sectors.
> + *
> + * If bio_sectors(@bio) was less than or equal to @sectors, returns @bio
> + * unchanged.

This is no longer true.

> + * The newly allocated bio will point to @bio's bi_io_vec, if the split was on a
> + * bvec boundry; it is the caller's responsibility to ensure that @bio is not
> + * freed before the split.
> + */
> +struct bio *bio_split(struct bio *bio, int sectors,
> + gfp_t gfp, struct bio_set *bs)
> +{
...
> + if (bio_integrity(bio)) {
> + bio_integrity_clone(split, bio, gfp, bs);

What happens if bio_integrity_clone() fails?

> + bio_integrity_trim(split, 0, bio_sectors(split));
> + bio_integrity_trim(bio, bio_sectors(split), bio_sectors(bio));

I complained pretty loudly about this not being mentioned in the
description of this patch or the following one and there still is no
explanation. Come on, Kent.

> +static inline struct bio *bio_next_split(struct bio *bio, int sectors,
> + gfp_t gfp, struct bio_set *bs)
> +{
> + if (sectors >= bio_sectors(bio))
> + return bio;
> +
> + return bio_split(bio, sectors, gfp, bs);
> +}

First of all, I don't think this is necessary. In addition, this
doesn't have any comment explaining what it does and is never
mentioned or justified in patch description. Kent, you're repeating
the same mistakes even after being explicitly pointed out multiple
times. *Please* pay more attention.

Thanks.

--
tejun

--
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 10:11 AM.

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