Re: [PATCH v6 11/13] block: Rework bio_pair_split()

Hello,

On Fri, Aug 24, 2012 at 03:30:49AM -0700, Kent Overstreet wrote:

quoted

I complained about this in the last posting and in the previous patch.
Please respond.  Martin, are you okay with these integrity changes?

I did respond. I said more before, but in short the old
bio_integrity_split() only worked for single page bios, and thus wasn't
useful even as a starting point.

What my bio_split() does with the integrity is actually basically what
dm does (that's the only other place bio_integrity_trim()) is used).

And all those information should be in the patch description.  Imagine
this patch hitting some enterprise deployment, say four years after
now, and causing weird performance regression and someone succeeding
to bisect it down to this commit.  None of us would be remembering
much about this and could in fact be doing something completely
different.  Patches should at least try to record its visible
implications.

This type of information should be always attached to the patch not
only for future references but also to alert reviewers who aren't
necessarily following the whole series and its iterations.

quoted

Also, given that it's a pair split, it would be nice to somehow
indicate that ->split is the earlier half.  Before this change it was
pretty clear with ->bio1/2.

Fixed the comment to indicate that too.

Any chance we can do that with field name?

commit 5db0562e43099d06ed9488e7ae6bf0a0cc6ef5da
Author: Kent Overstreet [off-list ref]
Date:   Fri Aug 24 03:27:24 2012 -0700

    block: Rework bio_pair_split()
    
    This changes bio_pair_split() to use the new bio_split() underneath.
    
    This has two user visible changes: First, it's not restricted to single
    page bios anymore. Secondly, where before callers used bio1 and bio2 in
    struct bio_pair, now they use split and orig - this being a result of
    the new implementation.
    
    bio_integrity_split() is removed, as the old bio_pair_split() was the
    only user; the new bio_split() uses bio_integrity_clone/trim much like
    the dm code does.

      "which may affect XXX in YYY cases but I think it's okay because ZZZ."

If you aren't that sure,

      "which may affect AAA but I don't think that's a big deal
       because BBB.  This definitely requires Martin's ack."

Thanks.

-- 
tejun