Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 11 Jun 2020 11:20:29 -0700
From:   Matthew Wilcox <willy@infradead.org>
To:     linux-fsdevel@vger.kernel.org
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 20/51] block: Add bio_for_each_thp_segment_all
Message-ID: <20200611182029.GC8681@bombadil.infradead.org>
References: <20200610201345.13273-1-willy@infradead.org>
 <20200610201345.13273-21-willy@infradead.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200610201345.13273-21-willy@infradead.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Jun 10, 2020 at 01:13:14PM -0700, Matthew Wilcox wrote:
> +static inline void bvec_thp_advance(const struct bio_vec *bvec,
> +				struct bvec_iter_all *iter_all)
> +{
> +	struct bio_vec *bv = &iter_all->bv;
> +	unsigned int page_size = thp_size(bvec->bv_page);
> +
> +	if (iter_all->done) {
> +		bv->bv_page += thp_nr_pages(bv->bv_page);
> +		bv->bv_offset = 0;
> +	} else {
> +		BUG_ON(bvec->bv_offset >= page_size);
> +		bv->bv_page = bvec->bv_page;
> +		bv->bv_offset = bvec->bv_offset & (page_size - 1);
> +	}
> +	bv->bv_len = min(page_size - bv->bv_offset,
> +			 bvec->bv_len - iter_all->done);
> +	iter_all->done += bv->bv_len;
> +
> +	if (iter_all->done == bvec->bv_len) {
> +		iter_all->idx++;
> +		iter_all->done = 0;
> +	}
> +}

If, for example, we have an order-2 page followed by two order-0 pages
(thanks, generic/127!) in the bvec, we'll end up skipping the third
page because we calculate the size based on bvec->bv_page instead of
bv->bv_page.

+++ b/include/linux/bvec.h
@@ -166,15 +166,19 @@ static inline void bvec_thp_advance(const struct bio_vec *bvec,
                                struct bvec_iter_all *iter_all)
 {
        struct bio_vec *bv = &iter_all->bv;
-       unsigned int page_size = thp_size(bvec->bv_page);
+       unsigned int page_size;
 
        if (iter_all->done) {
                bv->bv_page += thp_nr_pages(bv->bv_page);
+               page_size = thp_size(bv->bv_page);
                bv->bv_offset = 0;
        } else {
-               BUG_ON(bvec->bv_offset >= page_size);
-               bv->bv_page = bvec->bv_page;
-               bv->bv_offset = bvec->bv_offset & (page_size - 1);
+               bv->bv_page = thp_head(bvec->bv_page +
+                               (bvec->bv_offset >> PAGE_SHIFT));
+               page_size = thp_size(bv->bv_page);
+               bv->bv_offset = bvec->bv_offset -
+                               (bv->bv_page - bvec->bv_page) * PAGE_SIZE;
+               BUG_ON(bv->bv_offset >= page_size);
        }
        bv->bv_len = min(page_size - bv->bv_offset,
                         bvec->bv_len - iter_all->done);

The previous code also wasn't handling the case fixed in 6bedf00e55e5
where a split bio might end up splitting a bvec.  That BUG_ON can probably
come out after a few months of testing.