No way to break bio_for_each_segment_all() macro?

Commit Message

Qu Wenruo April 6, 2019, 1:53 a.m. UTC

Hi,

I'm looking into a strange behavior that we can't break
bio_for_each_segment_all() after commit 6dc4f100c175 ("block: allow
bio_for_each_segment_all() to iterate over multi-page bvec").

It's screwing up all bio_for_each_segment_all() call with error out branch.


There is one relatively easy to trigger setup.

Here is my kernel branch, which is based on David Sterb's misc-next branch:
https://github.com/adam900710/linux/tree/tree_checker_testing

It's just two commits ahead.

Then some debug diff:


Straightforward, if we break, we should have err == ret.

Then run fstests btrfs/151, which will trigger a false alert in
tree-checker:

  BTRFS critical (device dm-1): corrupt leaf: root=3 block=570572800
slot=1 devid=1 invalid total bytes: have 0
  BTRFS error (device dm-1): block=570572800 write time tree block
corruption detected
  breaking out with ret=-117
  err=-117 out, but ret=0

So it looks like the break line doens't really break, but continue
executing.

Thanks,
Qu

Comments

Al Viro April 6, 2019, 2:01 a.m. UTC | #1

On Sat, Apr 06, 2019 at 09:53:07AM +0800, Qu Wenruo wrote:
> Hi,
> 
> I'm looking into a strange behavior that we can't break
> bio_for_each_segment_all() after commit 6dc4f100c175 ("block: allow
> bio_for_each_segment_all() to iterate over multi-page bvec").
> 
> It's screwing up all bio_for_each_segment_all() call with error out branch.

>         bio_for_each_segment_all(bvec, bio, i, iter_all) {
>                 root = BTRFS_I(bvec->bv_page->mapping->host)->root;
>                 ret = csum_dirty_buffer(root->fs_info, bvec->bv_page);
> -               if (ret)
> +               if (ret) {
> +                       err = ret;
> +                       pr_info("breaking out with ret=%d\n", ret);
>                         break;
> +               }
>         }
> 
> +       if (err)
> +               pr_info("err=%d out, but ret=%d\n",err, ret);
>         return errno_to_blk_status(ret);
>  }
> 
> Straightforward, if we break, we should have err == ret.
> 
> Then run fstests btrfs/151, which will trigger a false alert in
> tree-checker:
> 
>   BTRFS critical (device dm-1): corrupt leaf: root=3 block=570572800
> slot=1 devid=1 invalid total bytes: have 0
>   BTRFS error (device dm-1): block=570572800 write time tree block
> corruption detected
>   breaking out with ret=-117
>   err=-117 out, but ret=0
> 
> So it looks like the break line doens't really break, but continue
> executing.

It expands to for-inside-for since that commit, so break only takes you
out of the inner loop...

No comments on desirability of such macros - personally, I prefer to
avoid those, but I'm not stepping into that holy war...

Qu Wenruo April 6, 2019, 2:09 a.m. UTC | #2

On 2019/4/6 上午10:01, Al Viro wrote:
> On Sat, Apr 06, 2019 at 09:53:07AM +0800, Qu Wenruo wrote:
>> Hi,
>>
>> I'm looking into a strange behavior that we can't break
>> bio_for_each_segment_all() after commit 6dc4f100c175 ("block: allow
>> bio_for_each_segment_all() to iterate over multi-page bvec").
>>
>> It's screwing up all bio_for_each_segment_all() call with error out branch.
> 
>>         bio_for_each_segment_all(bvec, bio, i, iter_all) {
>>                 root = BTRFS_I(bvec->bv_page->mapping->host)->root;
>>                 ret = csum_dirty_buffer(root->fs_info, bvec->bv_page);
>> -               if (ret)
>> +               if (ret) {
>> +                       err = ret;
>> +                       pr_info("breaking out with ret=%d\n", ret);
>>                         break;
>> +               }
>>         }
>>
>> +       if (err)
>> +               pr_info("err=%d out, but ret=%d\n",err, ret);
>>         return errno_to_blk_status(ret);
>>  }
>>
>> Straightforward, if we break, we should have err == ret.
>>
>> Then run fstests btrfs/151, which will trigger a false alert in
>> tree-checker:
>>
>>   BTRFS critical (device dm-1): corrupt leaf: root=3 block=570572800
>> slot=1 devid=1 invalid total bytes: have 0
>>   BTRFS error (device dm-1): block=570572800 write time tree block
>> corruption detected
>>   breaking out with ret=-117
>>   err=-117 out, but ret=0
>>
>> So it looks like the break line doens't really break, but continue
>> executing.
> 
> It expands to for-inside-for since that commit, so break only takes you
> out of the inner loop...
> 
> No comments on desirability of such macros - personally, I prefer to
> avoid those, but I'm not stepping into that holy war...

But it's a regression at least, not only for btrfs, but at least another
caller:
  https://elixir.bootlin.com/linux/v5.1-rc3/source/block/bio.c#L1134

At least it's a surprise for some old code.

Anyway, I'll fix the problem in btrfs.

Thanks,
Qu

Ming Lei April 6, 2019, 1:26 p.m. UTC | #3

On Sat, Apr 06, 2019 at 09:53:07AM +0800, Qu Wenruo wrote:
> Hi,
> 
> I'm looking into a strange behavior that we can't break
> bio_for_each_segment_all() after commit 6dc4f100c175 ("block: allow
> bio_for_each_segment_all() to iterate over multi-page bvec").
> 
> It's screwing up all bio_for_each_segment_all() call with error out branch.
> 
> 
> There is one relatively easy to trigger setup.
> 
> Here is my kernel branch, which is based on David Sterb's misc-next branch:
> https://github.com/adam900710/linux/tree/tree_checker_testing
> 
> It's just two commits ahead.
> 
> Then some debug diff:
> 
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index c2c0640aea55..05c880a5254b 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -851,16 +851,22 @@ static blk_status_t btree_csum_one_bio(struct bio
> *bio)
>         struct bio_vec *bvec;
>         struct btrfs_root *root;
>         int i, ret = 0;
> +       int err = 0;
>         struct bvec_iter_all iter_all;
> 
>         ASSERT(!bio_flagged(bio, BIO_CLONED));
>         bio_for_each_segment_all(bvec, bio, i, iter_all) {
>                 root = BTRFS_I(bvec->bv_page->mapping->host)->root;
>                 ret = csum_dirty_buffer(root->fs_info, bvec->bv_page);
> -               if (ret)
> +               if (ret) {
> +                       err = ret;
> +                       pr_info("breaking out with ret=%d\n", ret);
>                         break;
> +               }
>         }
> 
> +       if (err)
> +               pr_info("err=%d out, but ret=%d\n",err, ret);
>         return errno_to_blk_status(ret);
>  }
> 
> Straightforward, if we break, we should have err == ret.
>

Thanks for the report, and it should be easy to fix, will post patch
soon.

Thanks,
Ming

Ming Lei April 6, 2019, 1:47 p.m. UTC | #4

On Sat, Apr 06, 2019 at 09:53:07AM +0800, Qu Wenruo wrote:
> Hi,
> 
> I'm looking into a strange behavior that we can't break
> bio_for_each_segment_all() after commit 6dc4f100c175 ("block: allow
> bio_for_each_segment_all() to iterate over multi-page bvec").
> 
> It's screwing up all bio_for_each_segment_all() call with error out branch.
> 

Please test the following patch:

--

diff --git a/include/linux/bio.h b/include/linux/bio.h
index bb6090aa165d..7bd7e64e02f8 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -120,19 +120,15 @@ static inline bool bio_full(struct bio *bio)
 	return bio->bi_vcnt >= bio->bi_max_vecs;
 }
 
-#define mp_bvec_for_each_segment(bv, bvl, i, iter_all)			\
-	for (bv = bvec_init_iter_all(&iter_all);			\
-		(iter_all.done < (bvl)->bv_len) &&			\
-		(mp_bvec_next_segment((bvl), &iter_all), 1);		\
-		iter_all.done += bv->bv_len, i += 1)
-
 /*
  * drivers should _never_ use the all version - the bio may have been split
  * before it got to the driver and the driver won't own all of it
  */
-#define bio_for_each_segment_all(bvl, bio, i, iter_all)		\
-	for (i = 0, iter_all.idx = 0; iter_all.idx < (bio)->bi_vcnt; iter_all.idx++)	\
-		mp_bvec_for_each_segment(bvl, &((bio)->bi_io_vec[iter_all.idx]), i, iter_all)
+#define bio_for_each_segment_all(bvl, bio, i, iter_all)			\
+	for (i = 0, bvl = bvec_init_iter_all(&iter_all);		\
+		iter_all.idx < (bio)->bi_vcnt &&			\
+		(mp_bvec_advance(&((bio)->bi_io_vec[iter_all.idx]),	\
+				 &iter_all), 1); i++)
 
 static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
 				    unsigned bytes)
diff --git a/include/linux/bvec.h b/include/linux/bvec.h
index f6275c4da13a..6e4996dfc847 100644
--- a/include/linux/bvec.h
+++ b/include/linux/bvec.h
@@ -48,7 +48,7 @@ struct bvec_iter {
 struct bvec_iter_all {
 	struct bio_vec	bv;
 	int		idx;
-	unsigned	done;
+	unsigned	bv_done;
 };
 
 static inline struct page *bvec_nth_page(struct page *page, int idx)
@@ -145,18 +145,18 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
 
 static inline struct bio_vec *bvec_init_iter_all(struct bvec_iter_all *iter_all)
 {
-	iter_all->bv.bv_page = NULL;
-	iter_all->done = 0;
+	iter_all->bv_done = 0;
+	iter_all->idx = 0;
 
 	return &iter_all->bv;
 }
 
-static inline void mp_bvec_next_segment(const struct bio_vec *bvec,
-					struct bvec_iter_all *iter_all)
+static inline void mp_bvec_advance(const struct bio_vec *bvec,
+				   struct bvec_iter_all *iter_all)
 {
 	struct bio_vec *bv = &iter_all->bv;
 
-	if (bv->bv_page) {
+	if (iter_all->bv_done) {
 		bv->bv_page = nth_page(bv->bv_page, 1);
 		bv->bv_offset = 0;
 	} else {
@@ -164,7 +164,13 @@ static inline void mp_bvec_next_segment(const struct bio_vec *bvec,
 		bv->bv_offset = bvec->bv_offset;
 	}
 	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
-			   bvec->bv_len - iter_all->done);
+			   bvec->bv_len - iter_all->bv_done);
+	iter_all->bv_done += bv->bv_len;
+
+	if (iter_all->bv_done == bvec->bv_len) {
+		iter_all->idx++;
+		iter_all->bv_done = 0;
+	}
 }
 
 /*

Thanks,
Ming

Qu Wenruo April 6, 2019, 2 p.m. UTC | #5

On 2019/4/6 下午9:47, Ming Lei wrote:
> On Sat, Apr 06, 2019 at 09:53:07AM +0800, Qu Wenruo wrote:
>> Hi,
>>
>> I'm looking into a strange behavior that we can't break
>> bio_for_each_segment_all() after commit 6dc4f100c175 ("block: allow
>> bio_for_each_segment_all() to iterate over multi-page bvec").
>>
>> It's screwing up all bio_for_each_segment_all() call with error out branch.
>>
> 
> Please test the following patch:

It works!

Without touching the handling in btrfs part, error can break out of that
bio_for_each_segment_all() loop, just as it used to do.

Thanks,
Qu

> --
> 
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index bb6090aa165d..7bd7e64e02f8 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -120,19 +120,15 @@ static inline bool bio_full(struct bio *bio)
>  	return bio->bi_vcnt >= bio->bi_max_vecs;
>  }
>  
> -#define mp_bvec_for_each_segment(bv, bvl, i, iter_all)			\
> -	for (bv = bvec_init_iter_all(&iter_all);			\
> -		(iter_all.done < (bvl)->bv_len) &&			\
> -		(mp_bvec_next_segment((bvl), &iter_all), 1);		\
> -		iter_all.done += bv->bv_len, i += 1)
> -
>  /*
>   * drivers should _never_ use the all version - the bio may have been split
>   * before it got to the driver and the driver won't own all of it
>   */
> -#define bio_for_each_segment_all(bvl, bio, i, iter_all)		\
> -	for (i = 0, iter_all.idx = 0; iter_all.idx < (bio)->bi_vcnt; iter_all.idx++)	\
> -		mp_bvec_for_each_segment(bvl, &((bio)->bi_io_vec[iter_all.idx]), i, iter_all)
> +#define bio_for_each_segment_all(bvl, bio, i, iter_all)			\
> +	for (i = 0, bvl = bvec_init_iter_all(&iter_all);		\
> +		iter_all.idx < (bio)->bi_vcnt &&			\
> +		(mp_bvec_advance(&((bio)->bi_io_vec[iter_all.idx]),	\
> +				 &iter_all), 1); i++)
>  
>  static inline void bio_advance_iter(struct bio *bio, struct bvec_iter *iter,
>  				    unsigned bytes)
> diff --git a/include/linux/bvec.h b/include/linux/bvec.h
> index f6275c4da13a..6e4996dfc847 100644
> --- a/include/linux/bvec.h
> +++ b/include/linux/bvec.h
> @@ -48,7 +48,7 @@ struct bvec_iter {
>  struct bvec_iter_all {
>  	struct bio_vec	bv;
>  	int		idx;
> -	unsigned	done;
> +	unsigned	bv_done;
>  };
>  
>  static inline struct page *bvec_nth_page(struct page *page, int idx)
> @@ -145,18 +145,18 @@ static inline bool bvec_iter_advance(const struct bio_vec *bv,
>  
>  static inline struct bio_vec *bvec_init_iter_all(struct bvec_iter_all *iter_all)
>  {
> -	iter_all->bv.bv_page = NULL;
> -	iter_all->done = 0;
> +	iter_all->bv_done = 0;
> +	iter_all->idx = 0;
>  
>  	return &iter_all->bv;
>  }
>  
> -static inline void mp_bvec_next_segment(const struct bio_vec *bvec,
> -					struct bvec_iter_all *iter_all)
> +static inline void mp_bvec_advance(const struct bio_vec *bvec,
> +				   struct bvec_iter_all *iter_all)
>  {
>  	struct bio_vec *bv = &iter_all->bv;
>  
> -	if (bv->bv_page) {
> +	if (iter_all->bv_done) {
>  		bv->bv_page = nth_page(bv->bv_page, 1);
>  		bv->bv_offset = 0;
>  	} else {
> @@ -164,7 +164,13 @@ static inline void mp_bvec_next_segment(const struct bio_vec *bvec,
>  		bv->bv_offset = bvec->bv_offset;
>  	}
>  	bv->bv_len = min_t(unsigned int, PAGE_SIZE - bv->bv_offset,
> -			   bvec->bv_len - iter_all->done);
> +			   bvec->bv_len - iter_all->bv_done);
> +	iter_all->bv_done += bv->bv_len;
> +
> +	if (iter_all->bv_done == bvec->bv_len) {
> +		iter_all->idx++;
> +		iter_all->bv_done = 0;
> +	}
>  }
>  
>  /*
> 
> Thanks,
> Ming
>

Patch

diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index c2c0640aea55..05c880a5254b 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -851,16 +851,22 @@  static blk_status_t btree_csum_one_bio(struct bio
*bio)
        struct bio_vec *bvec;
        struct btrfs_root *root;
        int i, ret = 0;
+       int err = 0;
        struct bvec_iter_all iter_all;

        ASSERT(!bio_flagged(bio, BIO_CLONED));
        bio_for_each_segment_all(bvec, bio, i, iter_all) {
                root = BTRFS_I(bvec->bv_page->mapping->host)->root;
                ret = csum_dirty_buffer(root->fs_info, bvec->bv_page);
-               if (ret)
+               if (ret) {
+                       err = ret;
+                       pr_info("breaking out with ret=%d\n", ret);
                        break;
+               }
        }

+       if (err)
+               pr_info("err=%d out, but ret=%d\n",err, ret);
        return errno_to_blk_status(ret);
 }