| Message ID | 20231120224058.2750705-2-kbusch@meta.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block integrity: directly map user space addresses | expand |
On Mon, Nov 20, 2023 at 02:40:54PM -0800, Keith Busch wrote: > diff --git a/include/linux/bvec.h b/include/linux/bvec.h > index 555aae5448ae4..9364c258513e0 100644 > --- a/include/linux/bvec.h > +++ b/include/linux/bvec.h > @@ -184,6 +184,12 @@ static inline void bvec_iter_advance_single(const struct bio_vec *bv, > ((bvl = bvec_iter_bvec((bio_vec), (iter))), 1); \ > bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) > > +#define for_each_mp_bvec(bvl, bio_vec, iter, start) \ > + for (iter = (start); \ > + (iter).bi_size && \ > + ((bvl = mp_bvec_iter_bvec((bio_vec), (iter))), 1); \ > + bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) Hope thjis isn't too much bike-shedding, but in the block layer we generally used _segment for the single page bvecs and just bvec for the not page size limited. This isn't the best naming either, but i wonder if it's worth to change the existing 4 callers and be consistent. (and maybe one or two of them doesn't want the limit anyway?) Otherwise this looks good to me.
On Mon, Nov 20, 2023 at 02:40:54PM -0800, Keith Busch wrote: > From: Keith Busch <kbusch@kernel.org> > > Some bio_vec iterators can handle physically contiguous memory and have > no need to split bvec consideration on page boundaries. Then I am wondering why this helper is needed, and you can use each bvec directly, which is supposed to be physically contiguous. > > Signed-off-by: Keith Busch <kbusch@kernel.org> > --- > include/linux/bvec.h | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/include/linux/bvec.h b/include/linux/bvec.h > index 555aae5448ae4..9364c258513e0 100644 > --- a/include/linux/bvec.h > +++ b/include/linux/bvec.h > @@ -184,6 +184,12 @@ static inline void bvec_iter_advance_single(const struct bio_vec *bv, > ((bvl = bvec_iter_bvec((bio_vec), (iter))), 1); \ > bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) > > +#define for_each_mp_bvec(bvl, bio_vec, iter, start) \ > + for (iter = (start); \ > + (iter).bi_size && \ > + ((bvl = mp_bvec_iter_bvec((bio_vec), (iter))), 1); \ > + bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) > + We already have bio_for_each_bvec() to iterate over (multipage)bvecs from bio. Thanks, Ming
On Tue, Nov 21, 2023 at 04:37:01PM +0800, Ming Lei wrote: > On Mon, Nov 20, 2023 at 02:40:54PM -0800, Keith Busch wrote: > > From: Keith Busch <kbusch@kernel.org> > > > > Some bio_vec iterators can handle physically contiguous memory and have > > no need to split bvec consideration on page boundaries. > > Then I am wondering why this helper is needed, and you can use each bvec > directly, which is supposed to be physically contiguous. It's just a helper function to iterate a generic bvec. > > diff --git a/include/linux/bvec.h b/include/linux/bvec.h > > index 555aae5448ae4..9364c258513e0 100644 > > --- a/include/linux/bvec.h > > +++ b/include/linux/bvec.h > > @@ -184,6 +184,12 @@ static inline void bvec_iter_advance_single(const struct bio_vec *bv, > > ((bvl = bvec_iter_bvec((bio_vec), (iter))), 1); \ > > bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) > > > > +#define for_each_mp_bvec(bvl, bio_vec, iter, start) \ > > + for (iter = (start); \ > > + (iter).bi_size && \ > > + ((bvl = mp_bvec_iter_bvec((bio_vec), (iter))), 1); \ > > + bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) > > + > > We already have bio_for_each_bvec() to iterate over (multipage)bvecs > from bio. Right, but we are not dealing with a bio here. We have a bip bvec instead, so can't use bio_for_each_bvec().
On Tue, Nov 21, 2023 at 08:49:45AM -0700, Keith Busch wrote: > On Tue, Nov 21, 2023 at 04:37:01PM +0800, Ming Lei wrote: > > On Mon, Nov 20, 2023 at 02:40:54PM -0800, Keith Busch wrote: > > > From: Keith Busch <kbusch@kernel.org> > > > > > > Some bio_vec iterators can handle physically contiguous memory and have > > > no need to split bvec consideration on page boundaries. > > > > Then I am wondering why this helper is needed, and you can use each bvec > > directly, which is supposed to be physically contiguous. > > It's just a helper function to iterate a generic bvec. I just look into patch 3 about the use, seems what you need is for_each_bvec_all(), which is safe & efficient to use when freeing the host data(bio or bip), but can't be used in split bio/bip, in which the generic iterator is needed. And you can open-code it in bio_integrity_unmap_user(): for (i = 0; i < bip->bip_vcnt; i++) { struct bio_vec *v = &bip->bip_vec[i]; ... } Thanks, Ming
On Wed, Nov 22, 2023 at 08:43:48AM +0800, Ming Lei wrote: > > And you can open-code it in bio_integrity_unmap_user(): > > for (i = 0; i < bip->bip_vcnt; i++) { > struct bio_vec *v = &bip->bip_vec[i]; > > ... > } That works for me. io_uring/rsrc.c does similar too, which I referenced when implementing this. I thought the macro might help make this optimisation more reachable for future use, but I don't need to introduce that with only the one user here.
diff --git a/include/linux/bvec.h b/include/linux/bvec.h index 555aae5448ae4..9364c258513e0 100644 --- a/include/linux/bvec.h +++ b/include/linux/bvec.h @@ -184,6 +184,12 @@ static inline void bvec_iter_advance_single(const struct bio_vec *bv, ((bvl = bvec_iter_bvec((bio_vec), (iter))), 1); \ bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) +#define for_each_mp_bvec(bvl, bio_vec, iter, start) \ + for (iter = (start); \ + (iter).bi_size && \ + ((bvl = mp_bvec_iter_bvec((bio_vec), (iter))), 1); \ + bvec_iter_advance_single((bio_vec), &(iter), (bvl).bv_len)) + /* for iterating one bio from start to end */ #define BVEC_ITER_ALL_INIT (struct bvec_iter) \ { \