| Message ID | 1350331769-14856-28-git-send-email-koverstreet@google.com (mailing list archive) |
|---|---|
| State | Deferred, archived |
| Headers | show |
On Mon, Oct 15, 2012 at 01:09:01PM -0700, Kent Overstreet wrote: > bio_integrity_split() seemed to be confusing pointers and arrays - > bip_vec in bio_integrity_payload was an array appended to the end of the > payload, so the bio_vecs in struct bio_pair should have come after the > bio_integrity_payload they're for. > > Fix it by making bip_vec a pointer to the inline vecs - a later patch is > going to make more use of this pointer. > > Signed-off-by: Kent Overstreet <koverstreet@google.com> > CC: Jens Axboe <axboe@kernel.dk> > CC: Martin K. Petersen <martin.petersen@oracle.com> This being an apparent bug, I think it would be better to create a patch to simply reorder bio_pair fields to fix it and mark it w/ Cc: stable and introduce bip_vec pointer separately. Other than that, looks good. Thanks.
On Fri, Oct 19, 2012 at 01:30:15PM -0700, Tejun Heo wrote: > On Mon, Oct 15, 2012 at 01:09:01PM -0700, Kent Overstreet wrote: > > bio_integrity_split() seemed to be confusing pointers and arrays - > > bip_vec in bio_integrity_payload was an array appended to the end of the > > payload, so the bio_vecs in struct bio_pair should have come after the > > bio_integrity_payload they're for. > > > > Fix it by making bip_vec a pointer to the inline vecs - a later patch is > > going to make more use of this pointer. > > > > Signed-off-by: Kent Overstreet <koverstreet@google.com> > > CC: Jens Axboe <axboe@kernel.dk> > > CC: Martin K. Petersen <martin.petersen@oracle.com> > > This being an apparent bug, I think it would be better to create a > patch to simply reorder bio_pair fields to fix it and mark it w/ Cc: > stable and introduce bip_vec pointer separately. Initially he had reordered the bio_pair fields but there were couple of issues. We could not assume that there is no padding between fields and secondly some of the initializtion of looked plain ugly. IMHO, introduction of bip_vec to fix this issue is small and cleaner then reordering fields. Thanks Vivek -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel
>>>>> "Vivek" == Vivek Goyal <vgoyal@redhat.com> writes:
Vivek> Initially he had reordered the bio_pair fields but there were
Vivek> couple of issues. We could not assume that there is no padding
Vivek> between fields and secondly some of the initializtion of looked
Vivek> plain ugly.
Vivek> IMHO, introduction of bip_vec to fix this issue is small and
Vivek> cleaner then reordering fields.
I agree.
Hey, guys. On Wed, Oct 24, 2012 at 12:34 PM, Martin K. Petersen <martin.petersen@oracle.com> wrote: >>>>>> "Vivek" == Vivek Goyal <vgoyal@redhat.com> writes: > > Vivek> Initially he had reordered the bio_pair fields but there were > Vivek> couple of issues. We could not assume that there is no padding > Vivek> between fields and secondly some of the initializtion of looked > Vivek> plain ugly. > > Vivek> IMHO, introduction of bip_vec to fix this issue is small and > Vivek> cleaner then reordering fields. > > I agree. If Martin is happy with it, I'm not gonna push it but putting the respective storage member after one containing vararray at the end is a legit way to allocate the area statically. As long as the storage field is marked as such and not accessed directly, it doesn't matter whether there's padding inbetween or not. Anyways, let's push this one to -stable then. Thanks.
>>>>> "Tejun" == Tejun Heo <tj@kernel.org> writes:
Tejun> If Martin is happy with it, I'm not gonna push it but putting the
Tejun> respective storage member after one containing vararray at the
Tejun> end is a legit way to allocate the area statically. As long as
Tejun> the storage field is marked as such and not accessed directly, it
Tejun> doesn't matter whether there's padding inbetween or not.
The embedded array is still at the end. Kent just added an explicit
pointer for use in the bio_pair corner case.
diff --git a/fs/bio-integrity.c b/fs/bio-integrity.c index a3f28f3..94fa1c5 100644 --- a/fs/bio-integrity.c +++ b/fs/bio-integrity.c @@ -112,6 +112,7 @@ struct bio_integrity_payload *bio_integrity_alloc(struct bio *bio, bip->bip_slab = idx; bip->bip_bio = bio; + bip->bip_vec = bip->bip_inline_vecs; bio->bi_integrity = bip; return bip; @@ -697,8 +698,8 @@ void bio_integrity_split(struct bio *bio, struct bio_pair *bp, int sectors) bp->iv1 = bip->bip_vec[0]; bp->iv2 = bip->bip_vec[0]; - bp->bip1.bip_vec[0] = bp->iv1; - bp->bip2.bip_vec[0] = bp->iv2; + bp->bip1.bip_vec = &bp->iv1; + bp->bip2.bip_vec = &bp->iv2; bp->iv1.bv_len = sectors * bi->tuple_size; bp->iv2.bv_offset += sectors * bi->tuple_size; diff --git a/include/linux/bio.h b/include/linux/bio.h index b31036f..81004fd 100644 --- a/include/linux/bio.h +++ b/include/linux/bio.h @@ -182,7 +182,9 @@ struct bio_integrity_payload { unsigned short bip_idx; /* current bip_vec index */ struct work_struct bip_work; /* I/O completion */ - struct bio_vec bip_vec[0]; /* embedded bvec array */ + + struct bio_vec *bip_vec; + struct bio_vec bip_inline_vecs[0];/* embedded bvec array */ }; #endif /* CONFIG_BLK_DEV_INTEGRITY */
bio_integrity_split() seemed to be confusing pointers and arrays - bip_vec in bio_integrity_payload was an array appended to the end of the payload, so the bio_vecs in struct bio_pair should have come after the bio_integrity_payload they're for. Fix it by making bip_vec a pointer to the inline vecs - a later patch is going to make more use of this pointer. 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 | 5 +++-- include/linux/bio.h | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-)