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

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

[Alberto Bertogli]

Hi!

I'm trying to add bio-integrity support to a device-mapper target I posted
a couple of days ago (http://lkml.org/lkml/2009/5/21/235).

While at it, I found that bio_integrity_clone() does not clone neither bip_buf
nor bip_size, which already copies the bvec, which should have the same data
because it's allocated in bio_integrity_prep().


>From a quick grep, it seems they're only used inside fs/bio-integrity.c, and
through there they get passed to the functions each block device registered
for integrity verification and generation, and used in bio_integrity_tag().

Also, bio_integrity_free() already takes into account that and does not free
bip_buf if it's freeing a cloned bio.


Is there any reason I'm missing why they shouldn't be copied in
bio_integrity_clone(), as illustrated in the following patch?

Thanks a lot,
		Alberto


------- 8< ------- 8< ------- 8< ------- 8< ------- 8< ------- 8< -------

From: Alberto Bertogli <albertito@blitiri.com.ar>
Date: Sun, 24 May 2009 01:02:08 -0300
Subject: [PATCH] bio-integrity: Copy bip_buf and bip_size in bio_integrity_clone()

Signed-off-by: Alberto Bertogli <albertito@blitiri.com.ar>
---
 fs/bio-integrity.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c
index 31c46a2..fa9457d 100644
--- a/fs/bio-integrity.c
+++ b/fs/bio-integrity.c
@@ -682,6 +682,9 @@ int bio_integrity_clone(struct bio *bio, struct bio *bio_src, gfp_t gfp_mask)
 	if (bip == NULL)
 		return -EIO;
 
+	bip->bip_buf = bip_src->bip_buf;
+	bip->bip_size = bip_src->bip_size;
+
 	memcpy(bip->bip_vec, bip_src->bip_vec,
 	       bip_src->bip_vcnt * sizeof(struct bio_vec));
 
-- 
1.6.2.2.646.gb214

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

[Alberto Bertogli]

On Sun, May 24, 2009 at 01:20:35AM -0300, Alberto Bertogli wrote:
> While at it, I found that bio_integrity_clone() does not clone neither bip_buf
> nor bip_size, which already copies the bvec, which should have the same data
> because it's allocated in bio_integrity_prep().

In case that makes sense, updating bip_buf and bip_size (if bip_buf is not
NULL) in bio_integrity_trim() does too, doesn't it?

Thanks a lot,
		Alberto

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

[Martin K. Petersen]

>>>>> "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.

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.

Let me know if you have questions...

-- 
Martin K. Petersen	Oracle Linux Engineering

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

[Alberto Bertogli]

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

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

[Martin K. Petersen]

>>>>> "Alberto" == Alberto Bertogli <albertito@blitiri.com.ar> writes:

Alberto> The case I was thinking about was something like a filesystem
Alberto> calling bio_integrity_get_tag() on a cloned bio, 

Filesystems never see cloned bios.


Alberto>  - What would be a decent name to use in struct blk_integrity
Alberto>    for a module such as mine? Is LINUX-DMCSUM-V0-CCITT
Alberto>    reasonable?

Yeah.  Doesn't matter much, really.  The original intent was simply to
capture the variations in the (hardware) protocols .


Alberto>  - How can I test the tag functions? From a quick grep I found
Alberto>    no in-tree users of bio_integrity_get/set_tag().

There are no users yet.

-- 
Martin K. Petersen	Oracle Linux Engineering