On Tue, Mar 10, 2020 at 05:33:19PM +0100, Christoph Hellwig wrote:
> On Mon, Mar 09, 2020 at 02:32:29PM -0700, Omar Sandoval wrote:
> > + /*
> > + * We need to validate each sector individually if the I/O was for
> > + * multiple sectors.
> > + */
> > + len = 0;
> > + for (i = 0; i < failed_bio->bi_vcnt; i++) {
> > + len += failed_bio->bi_io_vec[i].bv_len;
> > + if (len > inode->i_sb->s_blocksize) {
> > + need_validation = true;
> > + break;
> > + }
>
> So given that btrfs is the I/O submitter iterating over all bio_vecs
> should probably be fine. That being said I deeply dislike the idea
> of having code like this inside the file system. Maybe we can add
> a helper like:
>
> u32 bio_size_all(struct bio *bio)
> {
> u32 len, i;
>
> for (i = 0; i < bio->bi_vcnt; i++)
> len += bio->bi_io_vec[i].bv_len;
> return len;
> }
>
> in the core block code, with a kerneldoc comment documenting the
> exact use cases, and then use that?
That works for me. I was microoptimizing here since I can stop iterating
once I know that the bio is greater than one sector, but since this is
for the rare repair case, it really doesn't matter.
On the other hand, a bio_for_each_bvec_all() helper would feel less
intrusive into the bio guts and also support bailing early. I'm fine
either way. Thoughts?
