Re: [RFC PATCH] bio-integrity: Copy bip_buf and bip_size in bio_integrity_clone()

[Alberto Bertogli <albertito@blitiri.com.ar>]

On Mon, May 25, 2009 at 01:04:55AM -0400, Martin K. Petersen wrote:
> >>>>> "Alberto" == Alberto Bertogli <albertito@blitiri.com.ar> writes:
> 
> Alberto> While at it, I found that bio_integrity_clone() does not clone
> Alberto> neither bip_buf nor bip_size, which already copies the bvec,
> Alberto> which should have the same data because it's allocated in
> Alberto> bio_integrity_prep().
> 
> Alberto> Is there any reason I'm missing why they shouldn't be copied in
> Alberto> bio_integrity_clone(), as illustrated in the following patch?
> 
> Yes.  The bip_buf is strictly an internal housekeeping construct.  It
> contains a pointer to the kernel buffer that contains the protection
> information if the protection was added by the block layer itself (via
> bio_integrity_prep()).  However, that's not always the case.  The
> protection information may be passed down from a filesystem or
> (eventually) a userland application.  So the bip_buf is for use by the
> top-level of the block layer exclusively.  The bip_vec is what you want
> to use for accessing the actual protection information.
>
> Also, the cloned bio may be truncated or split.  In that case the
> bip_buf isn't going to match the data bvec.  So in any case blindly
> cloning bip_buf isn't going to work.
>
> Right now the integrity vector itself is cloned together with the bio
> because of the way the block layer works (advancing bvec offset for
> partial completion).  However, I'm brewing on a patch that gets rid of
> that so we can avoid cloning the vector and thus cut down on memory
> allocations as the I/O goes through each remapping layer.  With my patch
> in place the bip_vec becomes immutable and bip_buf will go away.

That makes sense, thanks for the explanation.

The case I was thinking about was something like a filesystem calling
bio_integrity_get_tag() on a cloned bio, since it depends on having a bip_buf
available. But if you're going to remove it altogether then it's a moot
question.


> I took a quick look at your DM patch last week and I didn't see any
> reason why it couldn't hook into the block integrity infrastructure.
> Take a look at drivers/scsi/sd_dif.c for clues on how to do that.

Thanks, I've already implemented it, and will post an updated patch soon, after
I clean it up a little.


> Let me know if you have questions...

Actually, I have two minor questions, if you don't mind:

 - What would be a decent name to use in struct blk_integrity for a module
   such as mine? Is LINUX-DMCSUM-V0-CCITT reasonable?
 - How can I test the tag functions? From a quick grep I found no in-tree
   users of bio_integrity_get/set_tag().

Thanks a lot,
		Alberto