diff mbox

4.5-rc iser issues

Message ID 56C088EA.1050901@dev.mellanox.co.il
State New, archived
Headers show

Commit Message

Sagi Grimberg Feb. 14, 2016, 2:02 p.m. UTC
>> I'm bisecting now, there are a couple of patches from Ming in
>> the area of the bio splitting code...
>>
>> CC'ing Ming, Linux-block and Linux-nvme as iser is identical to nvme
>> wrt the virtual boundary so I think nvme will break as well.
>
> Bisection reveals that this one is the culprit:
>
> commit 52cc6eead9095e2faf2ec7afc013aa3af1f01ac5
> Author: Ming Lei <ming.lei@canonical.com>
> Date:   Thu Sep 17 09:58:38 2015 -0600
>
>      block: blk-merge: fast-clone bio when splitting rw bios
>
>      biovecs has become immutable since v3.13, so it isn't necessary
>      to allocate biovecs for the new cloned bios, then we can save
>      one extra biovecs allocation/copy, and the allocation is often
>      not fixed-length and a bit more expensive.
>
>      For example, if the 'max_sectors_kb' of null blk's queue is set
>      as 16(32 sectors) via sysfs just for making more splits, this patch
>      can increase throught about ~70% in the sequential read test over
>      null_blk(direct io, bs: 1M).
>
>      Cc: Christoph Hellwig <hch@infradead.org>
>      Cc: Kent Overstreet <kent.overstreet@gmail.com>
>      Cc: Ming Lin <ming.l@ssi.samsung.com>
>      Cc: Dongsu Park <dpark@posteo.net>
>      Signed-off-by: Ming Lei <ming.lei@canonical.com>
>
>      This fixes a performance regression introduced by commit 54efd50bfd,
>      and allows us to take full advantage of the fact that we have
> immutable
>      bio_vecs. Hand applied, as it rejected violently with commit
>      5014c311baa2.
>
>      Signed-off-by: Jens Axboe <axboe@fb.com>
> --

Looks like there is a problem with bio_clone_fast()

This change makes the problem go away:
--
--

Any thoughts?
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Hellwig Feb. 14, 2016, 3:22 p.m. UTC | #1
Adding Ming to Cc.

But I don't think simply not cloning the biovecs is the right thing
to do in the end.  This must be something with the bvec iterators.

Full quote for Ming:

On Sun, Feb 14, 2016 at 04:02:18PM +0200, Sagi Grimberg wrote:
> 
> >>I'm bisecting now, there are a couple of patches from Ming in
> >>the area of the bio splitting code...
> >>
> >>CC'ing Ming, Linux-block and Linux-nvme as iser is identical to nvme
> >>wrt the virtual boundary so I think nvme will break as well.
> >
> >Bisection reveals that this one is the culprit:
> >
> >commit 52cc6eead9095e2faf2ec7afc013aa3af1f01ac5
> >Author: Ming Lei <ming.lei@canonical.com>
> >Date:   Thu Sep 17 09:58:38 2015 -0600
> >
> >     block: blk-merge: fast-clone bio when splitting rw bios
> >
> >     biovecs has become immutable since v3.13, so it isn't necessary
> >     to allocate biovecs for the new cloned bios, then we can save
> >     one extra biovecs allocation/copy, and the allocation is often
> >     not fixed-length and a bit more expensive.
> >
> >     For example, if the 'max_sectors_kb' of null blk's queue is set
> >     as 16(32 sectors) via sysfs just for making more splits, this patch
> >     can increase throught about ~70% in the sequential read test over
> >     null_blk(direct io, bs: 1M).
> >
> >     Cc: Christoph Hellwig <hch@infradead.org>
> >     Cc: Kent Overstreet <kent.overstreet@gmail.com>
> >     Cc: Ming Lin <ming.l@ssi.samsung.com>
> >     Cc: Dongsu Park <dpark@posteo.net>
> >     Signed-off-by: Ming Lei <ming.lei@canonical.com>
> >
> >     This fixes a performance regression introduced by commit 54efd50bfd,
> >     and allows us to take full advantage of the fact that we have
> >immutable
> >     bio_vecs. Hand applied, as it rejected violently with commit
> >     5014c311baa2.
> >
> >     Signed-off-by: Jens Axboe <axboe@fb.com>
> >--
> 
> Looks like there is a problem with bio_clone_fast()
> 
> This change makes the problem go away:
> --
> diff --git a/block/bio.c b/block/bio.c
> index dbabd48..5e93733 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1791,7 +1791,7 @@ struct bio *bio_split(struct bio *bio, int sectors,
>          * Discards need a mutable bio_vec to accommodate the payload
>          * required by the DSM TRIM and UNMAP commands.
>          */
> -       if (bio->bi_rw & REQ_DISCARD)
> +       if (1 || bio->bi_rw & REQ_DISCARD)
>                 split = bio_clone_bioset(bio, gfp, bs);
>         else
>                 split = bio_clone_fast(bio, gfp, bs);
> --
> 
> Any thoughts?
---end quoted text---
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/block/bio.c b/block/bio.c
index dbabd48..5e93733 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1791,7 +1791,7 @@  struct bio *bio_split(struct bio *bio, int sectors,
          * Discards need a mutable bio_vec to accommodate the payload
          * required by the DSM TRIM and UNMAP commands.
          */
-       if (bio->bi_rw & REQ_DISCARD)
+       if (1 || bio->bi_rw & REQ_DISCARD)
                 split = bio_clone_bioset(bio, gfp, bs);
         else
                 split = bio_clone_fast(bio, gfp, bs);