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.
> 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). Ummm.... this is
simply unacceptable and makes me a lot more suspicious about the
patchset. Please be explicit about changes you make. Peer-review
breaks down unless such trust can be maintained.
Thanks.
--
tejun
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
