Re: [PATCH 1/2 v3] block: change rq_integrity_vec to respect the iterator

[Mikulas Patocka <mpatocka@redhat.com>]

On Thu, 23 May 2024, Christoph Hellwig wrote:

> On Thu, May 23, 2024 at 06:54:47PM +0200, Mikulas Patocka wrote:
> > 
> > However, the function rq_integrity_vec has a bug - it returns the first
> > vector of the bio's metadata and completely disregards the metadata
> > iterator that was advanced when the bio was split. Thus, the second bio
> > uses the same metadata as the first bio and this leads to metadata
> > corruption.
> > 
> > This commit changes rq_integrity_vec, so that it calls mp_bvec_iter_bvec
> > instead of returning the first vector. mp_bvec_iter_bvec reads the
> > iterator and advances the vector by the iterator.
> 
> mp_bvec_iter_bvec does not advance the bvec_iter, it just uses the
> iter to build a bvec for the current position in the iter.
> 
> Also please fix the commit log to not use more than 73 characters,
> as-is it will be unreadable in git show output or email replies.

OK. I thought that the limit was 74.

> > -static inline struct bio_vec *rq_integrity_vec(struct request *rq)
> > +static inline struct bio_vec rq_integrity_vec(struct request *rq)
> >  {
> >  	if (WARN_ON_ONCE(queue_max_integrity_segments(rq->q) > 1))
> > +		return (struct bio_vec){ };
> > +	return mp_bvec_iter_bvec(rq->bio->bi_integrity->bip_vec,
> > +				 rq->bio->bi_integrity->bip_iter);
> 
> The queue_max_integrity_segments check can go away now.  Once you
> use the bvec_iter the function works just fine for multiple
> integrity segments and always returns the one at the current iter
> position.   That should preferably also documented in a comment.

Yes, I dropped this and I am resending a new version.

> (I'm also pretty sure I've already written this in reply to Anuj's
> version of the patch)

Mikulas