| Message ID | 20181115085306.9910-9-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block: support multi-page bvec | expand |
On Thu, Nov 15, 2018 at 04:52:55PM +0800, Ming Lei wrote: > BTRFS is the only user of this helper, so move this helper into > BTRFS, and implement it via bio_for_each_segment_all(), since > bio->bi_vcnt may not equal to number of pages after multipage bvec > is enabled. Shouldn't you also get rid of bio_pages_all() in this patch? > Cc: Dave Chinner <dchinner@redhat.com> > Cc: Kent Overstreet <kent.overstreet@gmail.com> > Cc: Mike Snitzer <snitzer@redhat.com> > Cc: dm-devel@redhat.com > Cc: Alexander Viro <viro@zeniv.linux.org.uk> > Cc: linux-fsdevel@vger.kernel.org > Cc: Shaohua Li <shli@kernel.org> > Cc: linux-raid@vger.kernel.org > Cc: linux-erofs@lists.ozlabs.org > Cc: David Sterba <dsterba@suse.com> > Cc: linux-btrfs@vger.kernel.org > Cc: Darrick J. Wong <darrick.wong@oracle.com> > Cc: linux-xfs@vger.kernel.org > Cc: Gao Xiang <gaoxiang25@huawei.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Theodore Ts'o <tytso@mit.edu> > Cc: linux-ext4@vger.kernel.org > Cc: Coly Li <colyli@suse.de> > Cc: linux-bcache@vger.kernel.org > Cc: Boaz Harrosh <ooo@electrozaur.com> > Cc: Bob Peterson <rpeterso@redhat.com> > Cc: cluster-devel@redhat.com > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > fs/btrfs/extent_io.c | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c > index 5d5965297e7e..874bb9aeebdc 100644 > --- a/fs/btrfs/extent_io.c > +++ b/fs/btrfs/extent_io.c > @@ -2348,6 +2348,18 @@ struct bio *btrfs_create_repair_bio(struct inode *inode, struct bio *failed_bio, > return bio; > } > > +static unsigned btrfs_bio_pages_all(struct bio *bio) > +{ > + unsigned i; > + struct bio_vec *bv; > + > + WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)); > + > + bio_for_each_segment_all(bv, bio, i) > + ; > + return i; > +} > + > /* > * this is a generic handler for readpage errors (default > * readpage_io_failed_hook). if other copies exist, read those and write back > @@ -2368,7 +2380,7 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset, > int read_mode = 0; > blk_status_t status; > int ret; > - unsigned failed_bio_pages = bio_pages_all(failed_bio); > + unsigned failed_bio_pages = btrfs_bio_pages_all(failed_bio); > > BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE); > > -- > 2.9.5 >
On Thu, Nov 15, 2018 at 04:52:55PM +0800, Ming Lei wrote: > BTRFS is the only user of this helper, so move this helper into > BTRFS, and implement it via bio_for_each_segment_all(), since > bio->bi_vcnt may not equal to number of pages after multipage bvec > is enabled. btrfs only uses the value to check if it is larger than 1. No amount of multipage bio merging should ever make bi_vcnt go from 0 to 1 or vice versa.
On Thu, Nov 15, 2018 at 04:23:56PM -0800, Omar Sandoval wrote: > On Thu, Nov 15, 2018 at 04:52:55PM +0800, Ming Lei wrote: > > BTRFS is the only user of this helper, so move this helper into > > BTRFS, and implement it via bio_for_each_segment_all(), since > > bio->bi_vcnt may not equal to number of pages after multipage bvec > > is enabled. > > Shouldn't you also get rid of bio_pages_all() in this patch? Good catch! thanks, Ming
On Fri, Nov 16, 2018 at 02:38:45PM +0100, Christoph Hellwig wrote: > On Thu, Nov 15, 2018 at 04:52:55PM +0800, Ming Lei wrote: > > BTRFS is the only user of this helper, so move this helper into > > BTRFS, and implement it via bio_for_each_segment_all(), since > > bio->bi_vcnt may not equal to number of pages after multipage bvec > > is enabled. > > btrfs only uses the value to check if it is larger than 1. No amount > of multipage bio merging should ever make bi_vcnt go from 0 to 1 or > vice versa. Could you explain a bit why? Suppose 2 physically continuous pages are added to this bio, .bi_vcnt can be 1 in case of multi-page bvec, but it is 2 in case of single-page bvec. Thanks, Ming
On Mon, Nov 19, 2018 at 04:19:24PM +0800, Ming Lei wrote: > On Fri, Nov 16, 2018 at 02:38:45PM +0100, Christoph Hellwig wrote: > > On Thu, Nov 15, 2018 at 04:52:55PM +0800, Ming Lei wrote: > > > BTRFS is the only user of this helper, so move this helper into > > > BTRFS, and implement it via bio_for_each_segment_all(), since > > > bio->bi_vcnt may not equal to number of pages after multipage bvec > > > is enabled. > > > > btrfs only uses the value to check if it is larger than 1. No amount > > of multipage bio merging should ever make bi_vcnt go from 0 to 1 or > > vice versa. > > Could you explain a bit why? > > Suppose 2 physically continuous pages are added to this bio, .bi_vcnt > can be 1 in case of multi-page bvec, but it is 2 in case of single-page > bvec. True, I did think of 0 vs 1. The magic here in btrfs still doesn't make much sense. The comments down in btrfs_check_repairable talk about sectors, so it might be a leftover from the blocksize == PAGE_SIZE days (assuming those patches actually got merged). I'd like to have the btrfs folks chime in, but in the end we should probably check if the bio was larger than a single sector here.
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c index 5d5965297e7e..874bb9aeebdc 100644 --- a/fs/btrfs/extent_io.c +++ b/fs/btrfs/extent_io.c @@ -2348,6 +2348,18 @@ struct bio *btrfs_create_repair_bio(struct inode *inode, struct bio *failed_bio, return bio; } +static unsigned btrfs_bio_pages_all(struct bio *bio) +{ + unsigned i; + struct bio_vec *bv; + + WARN_ON_ONCE(bio_flagged(bio, BIO_CLONED)); + + bio_for_each_segment_all(bv, bio, i) + ; + return i; +} + /* * this is a generic handler for readpage errors (default * readpage_io_failed_hook). if other copies exist, read those and write back @@ -2368,7 +2380,7 @@ static int bio_readpage_error(struct bio *failed_bio, u64 phy_offset, int read_mode = 0; blk_status_t status; int ret; - unsigned failed_bio_pages = bio_pages_all(failed_bio); + unsigned failed_bio_pages = btrfs_bio_pages_all(failed_bio); BUG_ON(bio_op(failed_bio) == REQ_OP_WRITE);
BTRFS is the only user of this helper, so move this helper into BTRFS, and implement it via bio_for_each_segment_all(), since bio->bi_vcnt may not equal to number of pages after multipage bvec is enabled. Cc: Dave Chinner <dchinner@redhat.com> Cc: Kent Overstreet <kent.overstreet@gmail.com> Cc: Mike Snitzer <snitzer@redhat.com> Cc: dm-devel@redhat.com Cc: Alexander Viro <viro@zeniv.linux.org.uk> Cc: linux-fsdevel@vger.kernel.org Cc: Shaohua Li <shli@kernel.org> Cc: linux-raid@vger.kernel.org Cc: linux-erofs@lists.ozlabs.org Cc: David Sterba <dsterba@suse.com> Cc: linux-btrfs@vger.kernel.org Cc: Darrick J. Wong <darrick.wong@oracle.com> Cc: linux-xfs@vger.kernel.org Cc: Gao Xiang <gaoxiang25@huawei.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Theodore Ts'o <tytso@mit.edu> Cc: linux-ext4@vger.kernel.org Cc: Coly Li <colyli@suse.de> Cc: linux-bcache@vger.kernel.org Cc: Boaz Harrosh <ooo@electrozaur.com> Cc: Bob Peterson <rpeterso@redhat.com> Cc: cluster-devel@redhat.com Signed-off-by: Ming Lei <ming.lei@redhat.com> --- fs/btrfs/extent_io.c | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)