| Message ID | 20180103135243.21169-1-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 > -- 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
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; }
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(-)