* [PATCH] btrfs: not a disk error if the bio_add_page fails @ 2018-01-03 8:07 Anand Jain 2018-01-03 11:34 ` Filipe Manana 2018-01-03 13:52 ` [PATCH v2] " Anand Jain 0 siblings, 2 replies; 7+ messages in thread From: Anand Jain @ 2018-01-03 8:07 UTC (permalink / raw) To: linux-btrfs bio_add_page() can fail for logical reasons as from the bio_add_page() comments:- 'This will only fail if either bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.' Don't inc the write error statistics for this. And set -EINVAL instead of -EIO. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/scrub.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index d766c73eb29a..8c1508a41f90 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -4618,15 +4618,15 @@ static int write_page_nocow(struct scrub_ctx *sctx, bio->bi_opf = REQ_OP_WRITE | REQ_SYNC; ret = bio_add_page(bio, page, PAGE_SIZE, 0); if (ret != PAGE_SIZE) { -leave_with_eio: bio_put(bio); + return -EINVAL; + } + + if (btrfsic_submit_bio_wait(bio)) { btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS); return -EIO; } - if (btrfsic_submit_bio_wait(bio)) - goto leave_with_eio; - bio_put(bio); return 0; } -- 2.15.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: not a disk error if the bio_add_page fails 2018-01-03 8:07 [PATCH] btrfs: not a disk error if the bio_add_page fails Anand Jain @ 2018-01-03 11:34 ` Filipe Manana 2018-01-03 13:53 ` Anand Jain 2018-01-03 13:52 ` [PATCH v2] " Anand Jain 1 sibling, 1 reply; 7+ messages in thread From: Filipe Manana @ 2018-01-03 11:34 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs On Wed, Jan 3, 2018 at 8:07 AM, Anand Jain <anand.jain@oracle.com> wrote: > bio_add_page() can fail for logical reasons as from the bio_add_page() > comments:- 'This will only fail if either > bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.' Don't inc the > write error statistics for this. And set -EINVAL instead of -EIO. > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > fs/btrfs/scrub.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index d766c73eb29a..8c1508a41f90 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -4618,15 +4618,15 @@ static int write_page_nocow(struct scrub_ctx *sctx, > bio->bi_opf = REQ_OP_WRITE | REQ_SYNC; > ret = bio_add_page(bio, page, PAGE_SIZE, 0); > if (ret != PAGE_SIZE) { > -leave_with_eio: > bio_put(bio); > + return -EINVAL; > + } > + > + if (btrfsic_submit_bio_wait(bio)) { > btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS); > return -EIO; > } You are now introducing a leak of the bio. Missing bio_put(bio) before returning. > > - if (btrfsic_submit_bio_wait(bio)) > - goto leave_with_eio; > - > bio_put(bio); > return 0; > } > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Filipe David Manana, “Whether you think you can, or you think you can't — you're right.” ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] btrfs: not a disk error if the bio_add_page fails 2018-01-03 11:34 ` Filipe Manana @ 2018-01-03 13:53 ` Anand Jain 0 siblings, 0 replies; 7+ messages in thread From: Anand Jain @ 2018-01-03 13:53 UTC (permalink / raw) To: fdmanana; +Cc: linux-btrfs On 01/03/2018 07:34 PM, Filipe Manana wrote: > On Wed, Jan 3, 2018 at 8:07 AM, Anand Jain <anand.jain@oracle.com> wrote: >> bio_add_page() can fail for logical reasons as from the bio_add_page() >> comments:- 'This will only fail if either >> bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.' Don't inc the >> write error statistics for this. And set -EINVAL instead of -EIO. >> >> Signed-off-by: Anand Jain <anand.jain@oracle.com> >> --- >> fs/btrfs/scrub.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >> index d766c73eb29a..8c1508a41f90 100644 >> --- a/fs/btrfs/scrub.c >> +++ b/fs/btrfs/scrub.c >> @@ -4618,15 +4618,15 @@ static int write_page_nocow(struct scrub_ctx *sctx, >> bio->bi_opf = REQ_OP_WRITE | REQ_SYNC; >> ret = bio_add_page(bio, page, PAGE_SIZE, 0); >> if (ret != PAGE_SIZE) { >> -leave_with_eio: >> bio_put(bio); >> + return -EINVAL; >> + } >> + >> + if (btrfsic_submit_bio_wait(bio)) { >> btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS); >> return -EIO; >> } > > You are now introducing a leak of the bio. Missing bio_put(bio) before > returning. Oops. My bad. Thanks. Now fixed in v2. - Anand >> >> - if (btrfsic_submit_bio_wait(bio)) >> - goto leave_with_eio; >> - >> bio_put(bio); >> return 0; >> } >> -- >> 2.15.0 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] btrfs: not a disk error if the bio_add_page fails 2018-01-03 8:07 [PATCH] btrfs: not a disk error if the bio_add_page fails Anand Jain 2018-01-03 11:34 ` Filipe Manana @ 2018-01-03 13:52 ` Anand Jain 2018-01-04 17:47 ` Liu Bo 1 sibling, 1 reply; 7+ messages in thread From: Anand Jain @ 2018-01-03 13:52 UTC (permalink / raw) To: linux-btrfs bio_add_page() can fail for logical reasons as from the bio_add_page() comments:- 'This will only fail if either bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.' Don't inc the write error statistics for this. And set -EINVAL instead of -EIO. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- v1->v2: Add missing bio_put(). Thanks Filipe. fs/btrfs/scrub.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c index ecfe3118d9dd..57ac39a3f046 100644 --- a/fs/btrfs/scrub.c +++ b/fs/btrfs/scrub.c @@ -4617,15 +4617,16 @@ static int write_page_nocow(struct scrub_ctx *sctx, bio->bi_opf = REQ_OP_WRITE | REQ_SYNC; ret = bio_add_page(bio, page, PAGE_SIZE, 0); if (ret != PAGE_SIZE) { -leave_with_eio: + bio_put(bio); + return -EINVAL; + } + + if (btrfsic_submit_bio_wait(bio)) { bio_put(bio); btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS); return -EIO; } - if (btrfsic_submit_bio_wait(bio)) - goto leave_with_eio; - bio_put(bio); return 0; } -- 2.15.0 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: not a disk error if the bio_add_page fails 2018-01-03 13:52 ` [PATCH v2] " Anand Jain @ 2018-01-04 17:47 ` Liu Bo 2018-01-04 18:50 ` Liu Bo 0 siblings, 1 reply; 7+ messages in thread From: Liu Bo @ 2018-01-04 17:47 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs On Wed, Jan 03, 2018 at 09:52:43PM +0800, Anand Jain wrote: > bio_add_page() can fail for logical reasons as from the bio_add_page() > comments:- 'This will only fail if either > bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.' Don't inc the > write error statistics for this. And set -EINVAL instead of -EIO. > It's correct to skip increasing counter, but why -EINVAL? (its caller only cares about if it's non-zero.) thanks, -liubo > Signed-off-by: Anand Jain <anand.jain@oracle.com> > --- > v1->v2: Add missing bio_put(). Thanks Filipe. > > fs/btrfs/scrub.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > index ecfe3118d9dd..57ac39a3f046 100644 > --- a/fs/btrfs/scrub.c > +++ b/fs/btrfs/scrub.c > @@ -4617,15 +4617,16 @@ static int write_page_nocow(struct scrub_ctx *sctx, > bio->bi_opf = REQ_OP_WRITE | REQ_SYNC; > ret = bio_add_page(bio, page, PAGE_SIZE, 0); > if (ret != PAGE_SIZE) { > -leave_with_eio: > + bio_put(bio); > + return -EINVAL; > + } > + > + if (btrfsic_submit_bio_wait(bio)) { > bio_put(bio); > btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS); > return -EIO; > } > > - if (btrfsic_submit_bio_wait(bio)) > - goto leave_with_eio; > - > bio_put(bio); > return 0; > } > -- > 2.15.0 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: not a disk error if the bio_add_page fails 2018-01-04 17:47 ` Liu Bo @ 2018-01-04 18:50 ` Liu Bo 2018-01-05 2:25 ` Anand Jain 0 siblings, 1 reply; 7+ messages in thread From: Liu Bo @ 2018-01-04 18:50 UTC (permalink / raw) To: Anand Jain; +Cc: linux-btrfs On Thu, Jan 04, 2018 at 09:47:56AM -0800, Liu Bo wrote: > On Wed, Jan 03, 2018 at 09:52:43PM +0800, Anand Jain wrote: > > bio_add_page() can fail for logical reasons as from the bio_add_page() > > comments:- 'This will only fail if either > > bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.' Don't inc the > > write error statistics for this. And set -EINVAL instead of -EIO. > > > > It's correct to skip increasing counter, but why -EINVAL? > (its caller only cares about if it's non-zero.) Well...there is no chance that bio_add_page() could return non-PAGE_SIZE as this bio has been just created, we can remove the error handling after bio_add_page(). thanks, -liubo > > thanks, > > -liubo > > Signed-off-by: Anand Jain <anand.jain@oracle.com> > > --- > > v1->v2: Add missing bio_put(). Thanks Filipe. > > > > fs/btrfs/scrub.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c > > index ecfe3118d9dd..57ac39a3f046 100644 > > --- a/fs/btrfs/scrub.c > > +++ b/fs/btrfs/scrub.c > > @@ -4617,15 +4617,16 @@ static int write_page_nocow(struct scrub_ctx *sctx, > > bio->bi_opf = REQ_OP_WRITE | REQ_SYNC; > > ret = bio_add_page(bio, page, PAGE_SIZE, 0); > > if (ret != PAGE_SIZE) { > > -leave_with_eio: > > + bio_put(bio); > > + return -EINVAL; > > + } > > + > > + if (btrfsic_submit_bio_wait(bio)) { > > bio_put(bio); > > btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS); > > return -EIO; > > } > > > > - if (btrfsic_submit_bio_wait(bio)) > > - goto leave_with_eio; > > - > > bio_put(bio); > > return 0; > > } > > -- > > 2.15.0 > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] btrfs: not a disk error if the bio_add_page fails 2018-01-04 18:50 ` Liu Bo @ 2018-01-05 2:25 ` Anand Jain 0 siblings, 0 replies; 7+ messages in thread From: Anand Jain @ 2018-01-05 2:25 UTC (permalink / raw) To: bo.li.liu; +Cc: linux-btrfs On 01/05/2018 02:50 AM, Liu Bo wrote: > On Thu, Jan 04, 2018 at 09:47:56AM -0800, Liu Bo wrote: >> On Wed, Jan 03, 2018 at 09:52:43PM +0800, Anand Jain wrote: >>> bio_add_page() can fail for logical reasons as from the bio_add_page() >>> comments:- 'This will only fail if either >>> bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio.' Don't inc the >>> write error statistics for this. And set -EINVAL instead of -EIO. >>> >> >> It's correct to skip increasing counter, but why -EINVAL? >> (its caller only cares about if it's non-zero.) > > Well...there is no chance that bio_add_page() could return > non-PAGE_SIZE as this bio has been just created, we can remove the > error handling after bio_add_page(). Ah. right, bio_add_page() can't fail in this thread, its not a bio clone, and its the first call to bio_add_page(). Will remove error handling part. Thanks, Anand > thanks, > -liubo >> >> thanks, >> >> -liubo >>> Signed-off-by: Anand Jain <anand.jain@oracle.com> >>> --- >>> v1->v2: Add missing bio_put(). Thanks Filipe. >>> >>> fs/btrfs/scrub.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/fs/btrfs/scrub.c b/fs/btrfs/scrub.c >>> index ecfe3118d9dd..57ac39a3f046 100644 >>> --- a/fs/btrfs/scrub.c >>> +++ b/fs/btrfs/scrub.c >>> @@ -4617,15 +4617,16 @@ static int write_page_nocow(struct scrub_ctx *sctx, >>> bio->bi_opf = REQ_OP_WRITE | REQ_SYNC; >>> ret = bio_add_page(bio, page, PAGE_SIZE, 0); >>> if (ret != PAGE_SIZE) { >>> -leave_with_eio: >>> + bio_put(bio); >>> + return -EINVAL; >>> + } >>> + >>> + if (btrfsic_submit_bio_wait(bio)) { >>> bio_put(bio); >>> btrfs_dev_stat_inc_and_print(dev, BTRFS_DEV_STAT_WRITE_ERRS); >>> return -EIO; >>> } >>> >>> - if (btrfsic_submit_bio_wait(bio)) >>> - goto leave_with_eio; >>> - >>> bio_put(bio); >>> return 0; >>> } >>> -- >>> 2.15.0 >>> >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- > To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-01-05 2:24 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-01-03 8:07 [PATCH] btrfs: not a disk error if the bio_add_page fails Anand Jain 2018-01-03 11:34 ` Filipe Manana 2018-01-03 13:53 ` Anand Jain 2018-01-03 13:52 ` [PATCH v2] " Anand Jain 2018-01-04 17:47 ` Liu Bo 2018-01-04 18:50 ` Liu Bo 2018-01-05 2:25 ` Anand Jain
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).