* No way to break bio_for_each_segment_all() macro?
@ 2019-04-06 1:53 Qu Wenruo
2019-04-06 2:01 ` Al Viro
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Qu Wenruo @ 2019-04-06 1:53 UTC (permalink / raw)
To: linux-btrfs, Linux FS Devel, linux-block, ming.lei
[-- Attachment #1.1: Type: text/plain, Size: 2011 bytes --]
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.
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: No way to break bio_for_each_segment_all() macro?
2019-04-06 1:53 No way to break bio_for_each_segment_all() macro? Qu Wenruo
@ 2019-04-06 2:01 ` Al Viro
2019-04-06 2:09 ` Qu Wenruo
2019-04-06 13:26 ` Ming Lei
2019-04-06 13:47 ` Ming Lei
2 siblings, 1 reply; 6+ messages in thread
From: Al Viro @ 2019-04-06 2:01 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, Linux FS Devel, linux-block, ming.lei
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...
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: No way to break bio_for_each_segment_all() macro?
2019-04-06 2:01 ` Al Viro
@ 2019-04-06 2:09 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2019-04-06 2:09 UTC (permalink / raw)
To: Al Viro; +Cc: linux-btrfs, Linux FS Devel, linux-block, ming.lei
[-- Attachment #1.1: Type: text/plain, Size: 2030 bytes --]
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
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: No way to break bio_for_each_segment_all() macro?
2019-04-06 1:53 No way to break bio_for_each_segment_all() macro? Qu Wenruo
2019-04-06 2:01 ` Al Viro
@ 2019-04-06 13:26 ` Ming Lei
2019-04-06 13:47 ` Ming Lei
2 siblings, 0 replies; 6+ messages in thread
From: Ming Lei @ 2019-04-06 13:26 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, Linux FS Devel, linux-block
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: No way to break bio_for_each_segment_all() macro?
2019-04-06 1:53 No way to break bio_for_each_segment_all() macro? Qu Wenruo
2019-04-06 2:01 ` Al Viro
2019-04-06 13:26 ` Ming Lei
@ 2019-04-06 13:47 ` Ming Lei
2019-04-06 14:00 ` Qu Wenruo
2 siblings, 1 reply; 6+ messages in thread
From: Ming Lei @ 2019-04-06 13:47 UTC (permalink / raw)
To: Qu Wenruo; +Cc: linux-btrfs, Linux FS Devel, linux-block
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: No way to break bio_for_each_segment_all() macro?
2019-04-06 13:47 ` Ming Lei
@ 2019-04-06 14:00 ` Qu Wenruo
0 siblings, 0 replies; 6+ messages in thread
From: Qu Wenruo @ 2019-04-06 14:00 UTC (permalink / raw)
To: Ming Lei; +Cc: linux-btrfs, Linux FS Devel, linux-block
[-- Attachment #1.1: Type: text/plain, Size: 3562 bytes --]
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
>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-04-06 14:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-06 1:53 No way to break bio_for_each_segment_all() macro? Qu Wenruo
2019-04-06 2:01 ` Al Viro
2019-04-06 2:09 ` Qu Wenruo
2019-04-06 13:26 ` Ming Lei
2019-04-06 13:47 ` Ming Lei
2019-04-06 14:00 ` Qu Wenruo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).