Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1198559 > unrolled thread

Re: [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Ming Lin <mlin@kernel.org> - 2015-08-03 08:00 +0200
    Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Christoph Hellwig <hch@lst.de> - 2015-08-04 13:40 +0200
      Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Ming Lin <mlin@kernel.org> - 2015-08-05 08:10 +0200
        Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Christoph Hellwig <hch@lst.de> - 2015-08-07 09:40 +0200
          Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Ming Lin <mlin@kernel.org> - 2015-08-08 01:50 +0200
            Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Kent Overstreet <kent.overstreet@gmail.com> - 2015-08-08 02:40 +0200
              Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Ming Lin <mlin@kernel.org> - 2015-08-08 07:20 +0200
                Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Kent Overstreet <kent.overstreet@gmail.com> - 2015-08-08 07:30 +0200
                  Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Christoph Hellwig <hch@lst.de> - 2015-08-08 14:40 +0200
            Re: [dm-devel] [PATCH v5 01/11] block: make generic_make_request  handle arbitrarily sized bios Hannes Reinecke <hare@suse.de> - 2015-08-08 11:00 +0200
              Re: [dm-devel] [PATCH v5 01/11] block: make generic_make_request  handle arbitrarily sized bios Kent Overstreet <kent.overstreet@gmail.com> - 2015-08-08 11:10 +0200
                Re: [dm-devel] [PATCH v5 01/11] block: make generic_make_request  handle arbitrarily sized bios Hannes Reinecke <hare@suse.de> - 2015-08-13 08:10 +0200
      Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Kent Overstreet <kent.overstreet@gmail.com> - 2015-08-07 02:10 +0200
        Re: [PATCH v5 01/11] block: make generic_make_request handle  arbitrarily sized bios Christoph Hellwig <hch@lst.de> - 2015-08-07 09:40 +0200

#1198559 — Re: [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios

On Sat, 2015-08-01 at 12:33 -0400, Mike Snitzer wrote:
> On Sat, Aug 01 2015 at  2:58am -0400,
> Ming Lin <mlin@kernel.org> wrote:
> 
> > On Fri, 2015-07-31 at 17:38 -0400, Mike Snitzer wrote:
> > > 
> > > OK, once setup, to run the 2 tests in question directly you'd do
> > > something like:
> > > 
> > > dmtest run --suite thin-provisioning -n discard_a_fragmented_device
> > > 
> > > dmtest run --suite thin-provisioning -n discard_fully_provisioned_device_benchmark
> > > 
> > > Again, these tests pass without this patchset.
> > 
> > It's caused by patch 4.

Typo. I mean patch 5.

> > When discard size >=4G, the bio->bi_iter.bi_size overflows.
> 
> Thanks for tracking this down!

blkdev_issue_write_same() has same problem.

> 
> > Below is the new patch.
> > 
> > Christoph,
> > Could you also help to review it?
> > 
> > Now we still do "misaligned" check in blkdev_issue_discard().
> > So the same code in blk_bio_discard_split() was removed.
> 
> But I don't agree with this approach.  One of the most meaningful
> benefits of late bio splitting is the upper layers shouldn't _need_ to
> depend on the intermediate devices' queue_limits being stacked properly.
> Your solution to mix discard granularity/alignment checks at the upper
> layer(s) but then split based on max_discard_sectors at the lower layer
> defeats that benefit for discards.
> 
> This will translate to all intermediate layers that might split
> discards needing to worry about granularity/alignment
> too (e.g. how dm-thinp will have to care because it must generate
> discard mappings with associated bios based on how blocks were mapped to
> thinp).

I think the important thing is the late splitting for regular bio.
For discard/write_same bio, how about just don't do late splitting?

That is:
1. remove "PATCH 5: block: remove split code in blkdev_issue_discard"
2. Add below changes to PATCH 1

diff --git a/block/blk-merge.c b/block/blk-merge.c
index 1f5dfa0..90b085e 100644
--- a/block/blk-merge.c
+++ b/block/blk-merge.c
@@ -9,59 +9,6 @@
 
 #include "blk.h"
 
-static struct bio *blk_bio_discard_split(struct request_queue *q,
-					 struct bio *bio,
-					 struct bio_set *bs)
-{
-	unsigned int max_discard_sectors, granularity;
-	int alignment;
-	sector_t tmp;
-	unsigned split_sectors;
-
-	/* Zero-sector (unknown) and one-sector granularities are the same.  */
-	granularity = max(q->limits.discard_granularity >> 9, 1U);
-
-	max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9);
-	max_discard_sectors -= max_discard_sectors % granularity;
-
-	if (unlikely(!max_discard_sectors)) {
-		/* XXX: warn */
-		return NULL;
-	}
-
-	if (bio_sectors(bio) <= max_discard_sectors)
-		return NULL;
-
-	split_sectors = max_discard_sectors;
-
-	/*
-	 * If the next starting sector would be misaligned, stop the discard at
-	 * the previous aligned sector.
-	 */
-	alignment = (q->limits.discard_alignment >> 9) % granularity;
-
-	tmp = bio->bi_iter.bi_sector + split_sectors - alignment;
-	tmp = sector_div(tmp, granularity);
-
-	if (split_sectors > tmp)
-		split_sectors -= tmp;
-
-	return bio_split(bio, split_sectors, GFP_NOIO, bs);
-}
-
-static struct bio *blk_bio_write_same_split(struct request_queue *q,
-					    struct bio *bio,
-					    struct bio_set *bs)
-{
-	if (!q->limits.max_write_same_sectors)
-		return NULL;
-
-	if (bio_sectors(bio) <= q->limits.max_write_same_sectors)
-		return NULL;
-
-	return bio_split(bio, q->limits.max_write_same_sectors, GFP_NOIO, bs);
-}
-
 static struct bio *blk_bio_segment_split(struct request_queue *q,
 					 struct bio *bio,
 					 struct bio_set *bs)
@@ -129,10 +76,8 @@ void blk_queue_split(struct request_queue *q, struct bio **bio,
 {
 	struct bio *split;
 
-	if ((*bio)->bi_rw & REQ_DISCARD)
-		split = blk_bio_discard_split(q, *bio, bs);
-	else if ((*bio)->bi_rw & REQ_WRITE_SAME)
-		split = blk_bio_write_same_split(q, *bio, bs);
+	if ((*bio)->bi_rw & REQ_DISCARD || (*bio)->bi_rw & REQ_WRITE_SAME)
+		split = NULL;
 	else
 		split = blk_bio_segment_split(q, *bio, q->bio_split);
 

> 
> Also, it is unfortunate that IO that doesn't have a payload is being
> artificially split simply because bio->bi_iter.bi_size is 32bits.

Indeed.
Will it be possible to make it 64bits? I guess no.

> 
> Mike


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1199694

On Sun, Aug 02, 2015 at 10:58:22PM -0700, Ming Lin wrote:
> I think the important thing is the late splitting for regular bio.
> For discard/write_same bio, how about just don't do late splitting?

I'd hate having to special case them even more.  Especially as the
discard splitting is nasty and we really don't want to send giant
discards by default anyway (see Jens' patches to limit discard size
by default).

So I'd recommend to keep everything as-is, just make sure we don't
overflow bi_size.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1200428

On Tue, 2015-08-04 at 13:36 +0200, Christoph Hellwig wrote:
> On Sun, Aug 02, 2015 at 10:58:22PM -0700, Ming Lin wrote:
> > I think the important thing is the late splitting for regular bio.
> > For discard/write_same bio, how about just don't do late splitting?
> 
> I'd hate having to special case them even more.  Especially as the
> discard splitting is nasty and we really don't want to send giant
> discards by default anyway (see Jens' patches to limit discard size
> by default).
> 
> So I'd recommend to keep everything as-is, just make sure we don't
> overflow bi_size.

Did you mean to remove "PATCH 4 block: remove split code in
blkdev_issue_discard" or to keep it?

Which of below 2 solutions you prefer?

- Solution 1

remove splits in blkdev_issue_{discard,write_same} and keep
blk_bio_{discard,write_same}_split

But for blkdev_issue_discard(), it's not enough if only make sure
bi_size not overflow, for example, discard 4G

4G bytes = 8388608 sectors
UINT_MAX = 8388607 sectors

So blkdev_issue_discard() will send 2 discard bios.
First bio: sector 0 .. 8388606
Second bio: sector 8388607 .. 8388607

In this case, the 2 discard tests in device-mapper-test-suite still
fail, probably because the second bio start sector is not aligned with
discard_granularity.

So I have to take into account discard_granularity(assume 32 sectors),
then blkdev_issue_discard() will send 2 discard bios, as

First bio: sector 0 .. 8388575
Second bio: sector 8388576 .. 8388607

In this case, both discard tests passed.

- Solution 2

special case discard/write_same bios(You said you hate it).

That is to keep splits in blkdev_issue_{discard,write_same} and remove
blk_bio_{discard,write_same}_split

I think this is more clean way because blkdev_issue_{discard,write_same}
already make sure we don't overflow bi_size.

And blk_bio_{discard,write_same}_split are actually duplicated with the
splits in blkdev_issue_{discard,write_same}. It's OK to remove it.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202378

I'm for solution 3:

 - keep blk_bio_{discard,write_same}_split, but ensure we never built
   a > 4GB bio in blkdev_issue_{discard,write_same}.

Note that this isn't special casing, we can't build > 4GB bios for
data either, it's just implemented as a side effect right now instead
of checked explicitly.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203096

On Fri, 2015-08-07 at 09:30 +0200, Christoph Hellwig wrote:
> I'm for solution 3:
> 
>  - keep blk_bio_{discard,write_same}_split, but ensure we never built
>    a > 4GB bio in blkdev_issue_{discard,write_same}.

This has problem as I mentioned in solution 1.
We need to also make sure max discard size is of proper granularity.
See below example.

      4G: 8388608 sectors
UINT_MAX: 8388607 sectors

dm-thinp block size = default discard granularity = 128 sectors

blkdev_issue_discard(sector=0, nr_sectors=8388608)

1. Only ensure bi_size not overflow

It doesn't work.

[start_sector, end_sector]
[0, 8388607]
    [0, 8388606], then dm-thinp splits it to 2 bios
        [0, 8388479]
        [8388480, 8388606] ---> this has problem in process_discard_bio(),
                                because the discard size(7 sectors) covers less than a block(128 sectors)
    [8388607, 8388607] ---> same problem 

2. Ensure bi_size not overflow and max discard size is of proper granularity

It works.

[start_sector, end_sector]
[0, 8388607]
    [0, 8388479]
    [8388480, 8388607]


So how about below patch?

commit 1ca2ad977255efb3c339f4ca16fb798ed5ec54f7
Author: Ming Lin <ming.l@ssi.samsung.com>
Date:   Fri Aug 7 15:07:07 2015 -0700

    block: remove split code in blkdev_issue_{discard,write_same}
    
    The split code in blkdev_issue_{discard,write_same} can go away
    now that any driver that cares does the split. We have to make
    sure bio size doesn't overflow.
    
    For discard, we ensure max_discard_sectors is of the proper
    granularity. So if discard size > 4G, blkdev_issue_discard() always
    send multiple granularity requests to lower level, except that the
    last one may be not multiple granularity.
    
    Signed-off-by: Ming Lin <ming.l@ssi.samsung.com>
---
 block/blk-lib.c | 37 +++++++++----------------------------
 1 file changed, 9 insertions(+), 28 deletions(-)

diff --git a/block/blk-lib.c b/block/blk-lib.c
index 7688ee3..e178a07 100644
--- a/block/blk-lib.c
+++ b/block/blk-lib.c
@@ -44,7 +44,6 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	struct request_queue *q = bdev_get_queue(bdev);
 	int type = REQ_WRITE | REQ_DISCARD;
 	unsigned int max_discard_sectors, granularity;
-	int alignment;
 	struct bio_batch bb;
 	struct bio *bio;
 	int ret = 0;
@@ -58,18 +57,15 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 
 	/* Zero-sector (unknown) and one-sector granularities are the same.  */
 	granularity = max(q->limits.discard_granularity >> 9, 1U);
-	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
 
 	/*
-	 * Ensure that max_discard_sectors is of the proper
-	 * granularity, so that requests stay aligned after a split.
-	 */
-	max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9);
+	 * Ensure that max_discard_sectors doesn't overflow bi_size and is of
+	 * the proper granularity. So if discard size > 4G, blkdev_issue_discard()
+	 * always split and send multiple granularity requests to lower level,
+	 * except that the last one may be not multiple granularity.
+         */
+	max_discard_sectors = UINT_MAX >> 9;
 	max_discard_sectors -= max_discard_sectors % granularity;
-	if (unlikely(!max_discard_sectors)) {
-		/* Avoid infinite loop below. Being cautious never hurts. */
-		return -EOPNOTSUPP;
-	}
 
 	if (flags & BLKDEV_DISCARD_SECURE) {
 		if (!blk_queue_secdiscard(q))
@@ -84,7 +80,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 	blk_start_plug(&plug);
 	while (nr_sects) {
 		unsigned int req_sects;
-		sector_t end_sect, tmp;
+		sector_t end_sect;
 
 		bio = bio_alloc(gfp_mask, 1);
 		if (!bio) {
@@ -93,20 +89,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
 		}
 
 		req_sects = min_t(sector_t, nr_sects, max_discard_sectors);
-
-		/*
-		 * If splitting a request, and the next starting sector would be
-		 * misaligned, stop the discard at the previous aligned sector.
-		 */
 		end_sect = sector + req_sects;
-		tmp = end_sect;
-		if (req_sects < nr_sects &&
-		    sector_div(tmp, granularity) != alignment) {
-			end_sect = end_sect - alignment;
-			sector_div(end_sect, granularity);
-			end_sect = end_sect * granularity + alignment;
-			req_sects = end_sect - sector;
-		}
 
 		bio->bi_iter.bi_sector = sector;
 		bio->bi_end_io = bio_batch_end_io;
@@ -166,10 +149,8 @@ int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
 	if (!q)
 		return -ENXIO;
 
-	max_write_same_sectors = q->limits.max_write_same_sectors;
-
-	if (max_write_same_sectors == 0)
-		return -EOPNOTSUPP;
+	/* Ensure that max_write_same_sectors doesn't overflow bi_size */
+	max_write_same_sectors = UINT_MAX >> 9;
 
 	atomic_set(&bb.done, 1);
 	bb.flags = 1 << BIO_UPTODATE;


> 
> Note that this isn't special casing, we can't build > 4GB bios for
> data either, it's just implemented as a side effect right now instead
> of checked explicitly.




--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203111

On Fri, Aug 07, 2015 at 04:40:06PM -0700, Ming Lin wrote:
> 
> On Fri, 2015-08-07 at 09:30 +0200, Christoph Hellwig wrote:
> > I'm for solution 3:
> > 
> >  - keep blk_bio_{discard,write_same}_split, but ensure we never built
> >    a > 4GB bio in blkdev_issue_{discard,write_same}.
> 
> This has problem as I mentioned in solution 1.
> We need to also make sure max discard size is of proper granularity.
> See below example.
> 
>       4G: 8388608 sectors
> UINT_MAX: 8388607 sectors
> 
> dm-thinp block size = default discard granularity = 128 sectors
> 
> blkdev_issue_discard(sector=0, nr_sectors=8388608)
> 
> 1. Only ensure bi_size not overflow
> 
> It doesn't work.
> 
> [start_sector, end_sector]
> [0, 8388607]
>     [0, 8388606], then dm-thinp splits it to 2 bios
>         [0, 8388479]
>         [8388480, 8388606] ---> this has problem in process_discard_bio(),
>                                 because the discard size(7 sectors) covers less than a block(128 sectors)
>     [8388607, 8388607] ---> same problem 
> 
> 2. Ensure bi_size not overflow and max discard size is of proper granularity

Ideally we'd get upper layers out of the business of knowing about the queue
limits at all - that was the point of the patch series, after all.

Instead of using UINT_MAX, would it work to just make the max 1 << 31 sectors?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203173

On Fri, Aug 7, 2015 at 5:30 PM, Kent Overstreet
<kent.overstreet@gmail.com> wrote:
> On Fri, Aug 07, 2015 at 04:40:06PM -0700, Ming Lin wrote:
>>
>> On Fri, 2015-08-07 at 09:30 +0200, Christoph Hellwig wrote:
>> > I'm for solution 3:
>> >
>> >  - keep blk_bio_{discard,write_same}_split, but ensure we never built
>> >    a > 4GB bio in blkdev_issue_{discard,write_same}.
>>
>> This has problem as I mentioned in solution 1.
>> We need to also make sure max discard size is of proper granularity.
>> See below example.
>>
>>       4G: 8388608 sectors
>> UINT_MAX: 8388607 sectors
>>
>> dm-thinp block size = default discard granularity = 128 sectors
>>
>> blkdev_issue_discard(sector=0, nr_sectors=8388608)
>>
>> 1. Only ensure bi_size not overflow
>>
>> It doesn't work.
>>
>> [start_sector, end_sector]
>> [0, 8388607]
>>     [0, 8388606], then dm-thinp splits it to 2 bios
>>         [0, 8388479]
>>         [8388480, 8388606] ---> this has problem in process_discard_bio(),
>>                                 because the discard size(7 sectors) covers less than a block(128 sectors)
>>     [8388607, 8388607] ---> same problem
>>
>> 2. Ensure bi_size not overflow and max discard size is of proper granularity
>
> Ideally we'd get upper layers out of the business of knowing about the queue
> limits at all - that was the point of the patch series, after all.
>
> Instead of using UINT_MAX, would it work to just make the max 1 << 31 sectors?'

1 << 31 = 2G bytes = 0x400000 sectors.

Yes, that works as long as it's multiple of granularity.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203175

On Fri, Aug 07, 2015 at 10:17:43PM -0700, Ming Lin wrote:
> On Fri, Aug 7, 2015 at 5:30 PM, Kent Overstreet
> > Ideally we'd get upper layers out of the business of knowing about the queue
> > limits at all - that was the point of the patch series, after all.
> >
> > Instead of using UINT_MAX, would it work to just make the max 1 << 31 sectors?'
> 
> 1 << 31 = 2G bytes = 0x400000 sectors.
> 
> Yes, that works as long as it's multiple of granularity.

Is granularity required to be a power of two? One would hope, but looking at the
code that doesn't appear to be a requirement... ugh, that's terrible...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203264

On Fri, Aug 07, 2015 at 09:22:26PM -0800, Kent Overstreet wrote:
> Is granularity required to be a power of two? One would hope, but looking at the
> code that doesn't appear to be a requirement... ugh, that's terrible...

If devices have an odd granularity we'll just have to split more than
nessecary.  Let's hope no major hardware does that.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203226 — Re: [dm-devel] [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios

On 08/08/2015 01:40 AM, Ming Lin wrote:
> 
> On Fri, 2015-08-07 at 09:30 +0200, Christoph Hellwig wrote:
>> I'm for solution 3:
>>
>>  - keep blk_bio_{discard,write_same}_split, but ensure we never built
>>    a > 4GB bio in blkdev_issue_{discard,write_same}.
> 
> This has problem as I mentioned in solution 1.
> We need to also make sure max discard size is of proper granularity.
> See below example.
> 
>       4G: 8388608 sectors
> UINT_MAX: 8388607 sectors
> 
> dm-thinp block size = default discard granularity = 128 sectors
> 
> blkdev_issue_discard(sector=0, nr_sectors=8388608)
> 
> 1. Only ensure bi_size not overflow
> 
> It doesn't work.
> 
> [start_sector, end_sector]
> [0, 8388607]
>     [0, 8388606], then dm-thinp splits it to 2 bios
>         [0, 8388479]
>         [8388480, 8388606] ---> this has problem in process_discard_bio(),
>                                 because the discard size(7 sectors) covers less than a block(128 sectors)
>     [8388607, 8388607] ---> same problem 
> 
> 2. Ensure bi_size not overflow and max discard size is of proper granularity
> 
> It works.
> 
> [start_sector, end_sector]
> [0, 8388607]
>     [0, 8388479]
>     [8388480, 8388607]
> 
> 
> So how about below patch?
> 
> commit 1ca2ad977255efb3c339f4ca16fb798ed5ec54f7
> Author: Ming Lin <ming.l@ssi.samsung.com>
> Date:   Fri Aug 7 15:07:07 2015 -0700
> 
>     block: remove split code in blkdev_issue_{discard,write_same}
>     
>     The split code in blkdev_issue_{discard,write_same} can go away
>     now that any driver that cares does the split. We have to make
>     sure bio size doesn't overflow.
>     
>     For discard, we ensure max_discard_sectors is of the proper
>     granularity. So if discard size > 4G, blkdev_issue_discard() always
>     send multiple granularity requests to lower level, except that the
>     last one may be not multiple granularity.
>     
>     Signed-off-by: Ming Lin <ming.l@ssi.samsung.com>
> ---
>  block/blk-lib.c | 37 +++++++++----------------------------
>  1 file changed, 9 insertions(+), 28 deletions(-)
> 
> diff --git a/block/blk-lib.c b/block/blk-lib.c
> index 7688ee3..e178a07 100644
> --- a/block/blk-lib.c
> +++ b/block/blk-lib.c
> @@ -44,7 +44,6 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  	struct request_queue *q = bdev_get_queue(bdev);
>  	int type = REQ_WRITE | REQ_DISCARD;
>  	unsigned int max_discard_sectors, granularity;
> -	int alignment;
>  	struct bio_batch bb;
>  	struct bio *bio;
>  	int ret = 0;
> @@ -58,18 +57,15 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  
>  	/* Zero-sector (unknown) and one-sector granularities are the same.  */
>  	granularity = max(q->limits.discard_granularity >> 9, 1U);
> -	alignment = (bdev_discard_alignment(bdev) >> 9) % granularity;
>  
>  	/*
> -	 * Ensure that max_discard_sectors is of the proper
> -	 * granularity, so that requests stay aligned after a split.
> -	 */
> -	max_discard_sectors = min(q->limits.max_discard_sectors, UINT_MAX >> 9);
> +	 * Ensure that max_discard_sectors doesn't overflow bi_size and is of
> +	 * the proper granularity. So if discard size > 4G, blkdev_issue_discard()
> +	 * always split and send multiple granularity requests to lower level,
> +	 * except that the last one may be not multiple granularity.
> +         */
> +	max_discard_sectors = UINT_MAX >> 9;
>  	max_discard_sectors -= max_discard_sectors % granularity;
> -	if (unlikely(!max_discard_sectors)) {
> -		/* Avoid infinite loop below. Being cautious never hurts. */
> -		return -EOPNOTSUPP;
> -	}
>  
>  	if (flags & BLKDEV_DISCARD_SECURE) {
>  		if (!blk_queue_secdiscard(q))
> @@ -84,7 +80,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  	blk_start_plug(&plug);
>  	while (nr_sects) {
>  		unsigned int req_sects;
> -		sector_t end_sect, tmp;
> +		sector_t end_sect;
>  
>  		bio = bio_alloc(gfp_mask, 1);
>  		if (!bio) {
> @@ -93,20 +89,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,
>  		}
>  
>  		req_sects = min_t(sector_t, nr_sects, max_discard_sectors);
> -
> -		/*
> -		 * If splitting a request, and the next starting sector would be
> -		 * misaligned, stop the discard at the previous aligned sector.
> -		 */
>  		end_sect = sector + req_sects;
> -		tmp = end_sect;
> -		if (req_sects < nr_sects &&
> -		    sector_div(tmp, granularity) != alignment) {
> -			end_sect = end_sect - alignment;
> -			sector_div(end_sect, granularity);
> -			end_sect = end_sect * granularity + alignment;
> -			req_sects = end_sect - sector;
> -		}
>  
>  		bio->bi_iter.bi_sector = sector;
>  		bio->bi_end_io = bio_batch_end_io;
> @@ -166,10 +149,8 @@ int blkdev_issue_write_same(struct block_device *bdev, sector_t sector,
>  	if (!q)
>  		return -ENXIO;
>  
> -	max_write_same_sectors = q->limits.max_write_same_sectors;
> -
> -	if (max_write_same_sectors == 0)
> -		return -EOPNOTSUPP;
> +	/* Ensure that max_write_same_sectors doesn't overflow bi_size */
> +	max_write_same_sectors = UINT_MAX >> 9;
>  
>  	atomic_set(&bb.done, 1);
>  	bb.flags = 1 << BIO_UPTODATE;
> 
Wouldn't it be easier to move both max_write_same_sectors and
max_discard sectors to 64 bit (ie to type sector_t) and be done with the
overflow?
Seems to me this is far too much coding around self-imposed restrictions...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1203230 — Re: [dm-devel] [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios

On Sat, Aug 08, 2015 at 10:52:24AM +0200, Hannes Reinecke wrote:
> Wouldn't it be easier to move both max_write_same_sectors and
> max_discard sectors to 64 bit (ie to type sector_t) and be done with the
> overflow?
> Seems to me this is far too much coding around self-imposed restrictions...

It's bio->bi_iter.bi_size that would have to be increased to 64 bits. Which I
suppose wouldn't actually increase the size of struct bio (when sector_t is 64
bits), since struct bvec_iter has padding right now...
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1206528 — Re: [dm-devel] [PATCH v5 01/11] block: make generic_make_request handle arbitrarily sized bios

On 08/08/2015 11:02 AM, Kent Overstreet wrote:
> On Sat, Aug 08, 2015 at 10:52:24AM +0200, Hannes Reinecke wrote:
>> Wouldn't it be easier to move both max_write_same_sectors and
>> max_discard sectors to 64 bit (ie to type sector_t) and be done with the
>> overflow?
>> Seems to me this is far too much coding around self-imposed restrictions...
> 
> It's bio->bi_iter.bi_size that would have to be increased to 64 bits. Which I
> suppose wouldn't actually increase the size of struct bio (when sector_t is 64
> bits), since struct bvec_iter has padding right now...
> 
Which I guess we should be doing anyway.
Devices are getting larger and larger, so in the long run in need to
be moved to 64 bits.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202166

On Tue, Aug 04, 2015 at 01:36:26PM +0200, Christoph Hellwig wrote:
> On Sun, Aug 02, 2015 at 10:58:22PM -0700, Ming Lin wrote:
> > I think the important thing is the late splitting for regular bio.
> > For discard/write_same bio, how about just don't do late splitting?
> 
> I'd hate having to special case them even more.  Especially as the
> discard splitting is nasty and we really don't want to send giant
> discards by default anyway (see Jens' patches to limit discard size
> by default).

Why is it that we don't want to send giant discards? Is it a latency issue?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202380

On Thu, Aug 06, 2015 at 05:00:04PM -0700, Kent Overstreet wrote:
> Why is it that we don't want to send giant discards? Is it a latency issue?

Yes.  Take a look at the "Configurable max discard size" thread(s).
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web