Hi all,
raid5_mergeable_bvec() (q->merge_bvec_fn) checks for the rw-type and
makes decisions based on that if the bio is mergable or not. But so far
this value is only initialized on calling submit_bio(),
at least not by ext4 and xfs. I have not checked other file system so far.
Question 1: merge_bvec_fn is supposed to be removed, is is
still worth to fix the usage of merge_bvec_fn?
Question 2: If it is still supposed to be fixed, how do want to have
it? So far I have two patches for xfs and ext4 to set the rw type
directly there, but maybe bio_add_page should get an int rw_type
parameter and set bio->bi_rw itself?
diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
index d488f80..4cddc15 100644
--- a/fs/ext4/page-io.c
+++ b/fs/ext4/page-io.c
@@ -370,6 +370,7 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
bio->bi_bdev = bh->b_bdev;
bio->bi_end_io = ext4_end_bio;
bio->bi_private = ext4_get_io_end(io->io_end);
+ bio->bi_rw |= io->io_op;
io->io_bio = bio;
io->io_next_block = bh->b_blocknr;
return 0;
diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
index 71c8c9d..b48048f 100644
--- a/fs/xfs/xfs_aops.c
+++ b/fs/xfs/xfs_aops.c
@@ -401,7 +401,8 @@ xfs_submit_ioend_bio(
STATIC struct bio *
xfs_alloc_ioend_bio(
- struct buffer_head *bh)
+ struct buffer_head *bh,
+ struct writeback_control *wbc)
{
int nvecs = bio_get_nr_vecs(bh->b_bdev);
struct bio *bio = bio_alloc(GFP_NOIO, nvecs);
@@ -409,6 +410,7 @@ xfs_alloc_ioend_bio(
ASSERT(bio->bi_private == NULL);
bio->bi_sector = bh->b_blocknr * (bh->b_size >> 9);
bio->bi_bdev = bh->b_bdev;
+ bio->bi_rw |= (wbc->sync_mode == WB_SYNC_ALL) ? WRITE_SYNC : WRITE;
return bio;
}
@@ -511,7 +513,8 @@ xfs_submit_ioend(
if (!bio) {
retry:
- bio = xfs_alloc_ioend_bio(bh);
+ bio = xfs_alloc_ioend_bio(bh, wbc);
+
} else if (bh->b_blocknr != lastblock + 1) {
xfs_submit_ioend_bio(wbc, ioend, bio);
goto retry;
Thanks,
Bernd
[CCing linux-fsdevel instead of accidental fhgfs cc]
On 11/21/2013 07:00 PM, Bernd Schubert wrote:
> Hi all,
>
> raid5_mergeable_bvec() (q->merge_bvec_fn) checks for the rw-type and
> makes decisions based on that if the bio is mergable or not. But so far
> this value is only initialized on calling submit_bio(),
> at least not by ext4 and xfs. I have not checked other file system so far.
>
> Question 1: merge_bvec_fn is supposed to be removed, is is
> still worth to fix the usage of merge_bvec_fn?
>
> Question 2: If it is still supposed to be fixed, how do want to have
> it? So far I have two patches for xfs and ext4 to set the rw type
> directly there, but maybe bio_add_page should get an int rw_type
> parameter and set bio->bi_rw itself?
>
>
>
> diff --git a/fs/ext4/page-io.c b/fs/ext4/page-io.c
> index d488f80..4cddc15 100644
> --- a/fs/ext4/page-io.c
> +++ b/fs/ext4/page-io.c
> @@ -370,6 +370,7 @@ static int io_submit_init_bio(struct ext4_io_submit *io,
> bio->bi_bdev = bh->b_bdev;
> bio->bi_end_io = ext4_end_bio;
> bio->bi_private = ext4_get_io_end(io->io_end);
> + bio->bi_rw |= io->io_op;
> io->io_bio = bio;
> io->io_next_block = bh->b_blocknr;
> return 0;
>
> diff --git a/fs/xfs/xfs_aops.c b/fs/xfs/xfs_aops.c
> index 71c8c9d..b48048f 100644
> --- a/fs/xfs/xfs_aops.c
> +++ b/fs/xfs/xfs_aops.c
> @@ -401,7 +401,8 @@ xfs_submit_ioend_bio(
>
> STATIC struct bio *
> xfs_alloc_ioend_bio(
> - struct buffer_head *bh)
> + struct buffer_head *bh,
> + struct writeback_control *wbc)
> {
> int nvecs = bio_get_nr_vecs(bh->b_bdev);
> struct bio *bio = bio_alloc(GFP_NOIO, nvecs);
> @@ -409,6 +410,7 @@ xfs_alloc_ioend_bio(
> ASSERT(bio->bi_private == NULL);
> bio->bi_sector = bh->b_blocknr * (bh->b_size >> 9);
> bio->bi_bdev = bh->b_bdev;
> + bio->bi_rw |= (wbc->sync_mode == WB_SYNC_ALL) ? WRITE_SYNC : WRITE;
> return bio;
> }
>
> @@ -511,7 +513,8 @@ xfs_submit_ioend(
>
> if (!bio) {
> retry:
> - bio = xfs_alloc_ioend_bio(bh);
> + bio = xfs_alloc_ioend_bio(bh, wbc);
> +
> } else if (bh->b_blocknr != lastblock + 1) {
> xfs_submit_ioend_bio(wbc, ioend, bio);
> goto retry;
>
>
>
> Thanks,
> Bernd
>
While this is trivial to fix it's also fairly unexpected and easy
to get wrong for new callers. Neil, can you explain why you
desperately need it?
On Fri, 22 Nov 2013 07:36:18 -0800 Christoph Hellwig <[email protected]>
wrote:
> While this is trivial to fix it's also fairly unexpected and easy
> to get wrong for new callers. Neil, can you explain why you
> desperately need it?
Desperately? Not at all?
Need? Not really. This is just in RAID5 and merge_bvec_fn is purely an
optimisation for RAID5.
Limiting read BIOs to one chunk allows us to bypass the stripe-cache, so can
be good.
Limiting write BIOs is completely unnecessary so we currently don't bother.
So I have no objection to bvm->bi_rw being removed.
Thanks,
NeilBrown