block: make generic_make_request handle arbitrarily sized bios
Subsystems (23)
PATCH v5 1/11 block: make generic_make_request handle arbitrarily sized bios
From: Kent Overstreet <[email protected]>
The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.
But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete merge_bvec_fn and a bunch of other code.
We do this by adding calls to blk_queue_split() to the various make_request functions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call to blk_queue_bounce(); this means that blk_queue_split() and blk_recalc_rq_segments() don't need to be concerned with bouncing affecting segment merging.
Some make_request_fn() callbacks were simple enough to audit and verify they don't need blk_queue_split() calls. The skipped ones are:
* nfhd_make_request (arch/m68k/emu/nfblock.c)
* axon_ram_make_request (arch/powerpc/sysdev/axonram.c)
* simdisk_make_request (arch/xtensa/platforms/iss/simdisk.c)
* brd_make_request (ramdisk - drivers/block/brd.c)
* mtip_submit_request (drivers/block/mtip32xx/mtip32xx.c)
* loop_make_request
* null_queue_bio
* bcache's make_request fns
Some others are almost certainly safe to remove now, but will be left for future patches.
--- block/blk-core.c | 19 ++-- block/blk-merge.c | 159 ++++++++++++++++++++++++++-- block/blk-mq.c | 4 + block/blk-sysfs.c | 3 + drivers/block/drbd/drbd_req.c | 2 + drivers/block/pktcdvd.c | 6 +- drivers/block/ps3vram.c | 2 + drivers/block/rsxx/dev.c | 2 + drivers/block/umem.c | 2 + drivers/block/zram/zram_drv.c | 2 + drivers/md/dm.c | 2 + drivers/md/md.c | 2 + drivers/s390/block/dcssblk.c | 2 + drivers/s390/block/xpram.c | 2 + drivers/staging/lustre/lustre/llite/lloop.c | 2 + include/linux/blkdev.h | 3 + 16 files changed, 192 insertions(+), 22 deletions(-)
block/blk-core.c +9 -10
@@ -645,6 +645,10 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id)if (q->id <0 ) goto fail_q; + q->bio_split =bioset_create (BIO_POOL_SIZE ,0 ); +if (!q->bio_split) + goto fail_id; + q->backing_dev_info.ra_pages = (VM_MAX_READAHEAD *1024 ) /PAGE_CACHE_SIZE ; q->backing_dev_info.capabilities =BDI_CAP_CGROUP_WRITEBACK ;
@@ -653,7 +657,7 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id) err =bdi_init (&q->backing_dev_info);if (err) - goto fail_id; + goto fail_split;setup_timer (&q->backing_dev_info.laptop_mode_wb_timer, laptop_mode_timer_fn, (unsigned long ) q);
@@ -695,6 +699,8 @@ struct request_queue *blk_alloc_queue_node(gfp_t gfp_mask, int node_id) fail_bdi:bdi_destroy (&q->backing_dev_info); +fail_split: +bioset_free (q->bio_split); fail_id:ida_simple_remove (&blk_queue_ida, q->id); fail_q:
@@ -1612,6 +1618,8 @@ static void blk_queue_bio(struct request_queue *q, struct bio *bio)struct request *req;unsigned int request_count =0 ; +blk_queue_split (q, &bio, q->bio_split); +/* * low level driver can indicate that it wants pages above a * certain limit bounced to low memory (ie for highmem, or even
@@ -1832,15 +1840,6 @@ generic_make_request_checks(struct bio *bio) goto end_io; } -if (likely (bio_is_rw (bio) && - nr_sectors >queue_max_hw_sectors (q))) { -printk (KERN_ERR "bio too big device %s (%u > %u)\n" , -bdevname (bio->bi_bdev, b), -bio_sectors (bio), -queue_max_hw_sectors (q)); - goto end_io; - } - part = bio->bi_bdev->bd_part;if (should_fail_request (part, bio->bi_iter.bi_size) || should_fail_request(&part_to_disk(part)->part0,
block/blk-merge.c +149 -10
189 lines not on this page · raw message
block/blk-mq.c +4 -0
@@ -1291,6 +1291,8 @@ static void blk_mq_make_request(struct request_queue *q, struct bio *bio) blk_attempt_plug_merge(q, bio, &request_count, &same_queue_rq)) return; +blk_queue_split (q, &bio, q->bio_split); + rq =blk_mq_map_request (q, bio, &data);if (unlikely (!rq))return ;
@@ -1376,6 +1378,8 @@ static void blk_sq_make_request(struct request_queue *q, struct bio *bio) blk_attempt_plug_merge(q, bio, &request_count, NULL)) return; +blk_queue_split (q, &bio, q->bio_split); + rq =blk_mq_map_request (q, bio, &data);if (unlikely (!rq))return ;
block/blk-sysfs.c +3 -0
@@ -523,6 +523,9 @@ static void blk_release_queue(struct kobject *kobj)blk_trace_shutdown (q); +if (q->bio_split) +bioset_free (q->bio_split); +ida_simple_remove (&blk_queue_ida, q->id);call_rcu (&q->rcu_head, blk_free_queue_rcu); }
drivers/block/drbd/drbd_req.c +2 -0
@@ -1497,6 +1497,8 @@ void drbd_make_request(struct request_queue *q, struct bio *bio)struct drbd_device *device = (struct drbd_device *) q->queuedata;unsigned long start_jif; +blk_queue_split (q, &bio, q->bio_split); + start_jif = jiffies;/*
drivers/block/pktcdvd.c +4 -2
@@ -2447,6 +2447,10 @@ static void pkt_make_request(struct request_queue *q, struct bio *bio)char b[BDEVNAME_SIZE ];struct bio *split; +blk_queue_bounce (q, &bio); + +blk_queue_split (q, &bio, q->bio_split); + pd = q->queuedata;if (!pd) { pr_err("%s incorrect request queue\n",
@@ -2477,8 +2481,6 @@ static void pkt_make_request(struct request_queue *q, struct bio *bio) goto end_io; } -blk_queue_bounce (q, &bio); - do { sector_t zone = get_zone(bio->bi_iter.bi_sector, pd); sector_t last_zone = get_zone(bio_end_sector(bio) - 1, pd);
drivers/block/ps3vram.c +2 -0
@@ -605,6 +605,8 @@ static void ps3vram_make_request(struct request_queue *q, struct bio *bio)dev_dbg (&dev->core,"%s\n" , __func__); +blk_queue_split (q, &bio, q->bio_split); +spin_lock_irq (&priv->lock); busy = !bio_list_empty (&priv->list);bio_list_add (&priv->list, bio);
drivers/block/rsxx/dev.c +2 -0
@@ -148,6 +148,8 @@ static void rsxx_make_request(struct request_queue *q, struct bio *bio)struct rsxx_bio_meta *bio_meta;int st = -EINVAL ; +blk_queue_split (q, &bio, q->bio_split); +might_sleep ();if (!card)
drivers/block/umem.c +2 -0
@@ -531,6 +531,8 @@ static void mm_make_request(struct request_queue *q, struct bio *bio) (unsigned long long )bio->bi_iter.bi_sector, bio->bi_iter.bi_size); +blk_queue_split (q, &bio, q->bio_split); +spin_lock_irq (&card->lock); *card->biotail = bio; bio->bi_next =NULL ;
drivers/block/zram/zram_drv.c +2 -0
@@ -901,6 +901,8 @@ static void zram_make_request(struct request_queue *queue, struct bio *bio)if (unlikely (!zram_meta_get (zram))) goto error; +blk_queue_split (queue, &bio, queue->bio_split); +if (!valid_io_request (zram, bio->bi_iter.bi_sector, bio->bi_iter.bi_size)) {atomic64_inc (&zram->stats.invalid_io);
drivers/md/dm.c +2 -0
@@ -1800,6 +1800,8 @@ static void dm_make_request(struct request_queue *q, struct bio *bio) map =dm_get_live_table (md, &srcu_idx); +blk_queue_split (q, &bio, q->bio_split); +generic_start_io_acct (rw,bio_sectors (bio), &dm_disk (md)->part0);/* if we're suspended, we have to queue this io for later */
drivers/md/md.c +2 -0
@@ -257,6 +257,8 @@ static void md_make_request(struct request_queue *q, struct bio *bio)unsigned int sectors;int cpu; +blk_queue_split (q, &bio, q->bio_split); +if (mddev ==NULL || mddev->pers ==NULL || !mddev->ready) {bio_io_error (bio);
drivers/s390/block/dcssblk.c +2 -0
@@ -826,6 +826,8 @@ dcssblk_make_request(struct request_queue *q, struct bio *bio)unsigned long source_addr;unsigned long bytes_done; +blk_queue_split (q, &bio, q->bio_split); + bytes_done =0 ; dev_info = bio->bi_bdev->bd_disk->private_data;if (dev_info ==NULL )
drivers/s390/block/xpram.c +2 -0
@@ -190,6 +190,8 @@ static void xpram_make_request(struct request_queue *q, struct bio *bio)unsigned long page_addr;unsigned long bytes; +blk_queue_split (q, &bio, q->bio_split); +if ((bio->bi_iter.bi_sector &7 ) !=0 || (bio->bi_iter.bi_size &4095 ) !=0 )/* Request is not page-aligned. */
drivers/staging/lustre/lustre/llite/lloop.c +2 -0
@@ -340,6 +340,8 @@ static void loop_make_request(struct request_queue *q, struct bio *old_bio)int rw =bio_rw (old_bio);int inactive; +blk_queue_split (q, &old_bio, q->bio_split); +if (!lo) goto err;
include/linux/blkdev.h +3 -0
@@ -462,6 +462,7 @@ struct request_queue {struct blk_mq_tag_set *tag_set;struct list_head tag_set_list; +struct bio_set *bio_split; };#define QUEUE_FLAG_QUEUED 1/* uses generic tag queueing */
@@ -782,6 +783,8 @@ extern void blk_rq_unprep_clone(struct request *rq);extern int blk_insert_cloned_request (struct request_queue *q,struct request *rq);extern void blk_delay_queue (struct request_queue *,unsigned long ); +extern void blk_queue_split (struct request_queue *,struct bio **, +struct bio_set *);extern void blk_recount_segments (struct request_queue *,struct bio *);extern int scsi_verify_blk_ioctl (struct block_device *,unsigned int ); extern int scsi_cmd_blk_ioctl(struct block_device *, fmode_t,
Signature · 1 line
1.9.1
PATCH v5 2/11 block: simplify bio_add_page()
From: Kent Overstreet <[email protected]>
Since generic_make_request() can now handle arbitrary size bios, all we have to do is make sure the bvec array doesn't overflow. __bio_add_page() doesn't need to call ->merge_bvec_fn(), where we can get rid of unnecessary code paths.
Removing the call to ->merge_bvec_fn() is also fine, as no driver that implements support for BLOCK_PC commands even has a ->merge_bvec_fn() method.
[dpark: rebase and resolve merge conflicts, change a couple of comments,
make bio_add_page() warn once upon a cloned bio.]
--- block/bio.c | 135 +++++++++++++++++++++++++----------------------------------- 1 file changed, 55 insertions(+), 80 deletions(-)
block/bio.c +55 -80
@@ -714,9 +714,23 @@ int bio_get_nr_vecs(struct block_device *bdev) }EXPORT_SYMBOL (bio_get_nr_vecs); -static int __bio_add_page (struct request_queue *q,struct bio *bio,struct page - *page,unsigned int len,unsigned int offset, -unsigned int max_sectors) +/** +* bio_add_pc_page - attempt to add page to bio +* @q: the target queue +* @bio: destination bio +* @page: page to add +* @len: vec entry length +* @offset: vec entry offset +* +* Attempt to add a page to the bio_vec maplist. This can fail for a +* number of reasons, such as the bio being full or target block device +* limitations. The target block device must allow bio's up to PAGE_SIZE, +* so it is always possible to add a single page to an empty bio. +* +* This should only be used by REQ_PC bios. +*/ +int bio_add_pc_page (struct request_queue *q,struct bio *bio,struct page + *page,unsigned int len,unsigned int offset) {int retried_segments =0 ;struct bio_vec *bvec;
@@ -727,7 +741,7 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pageif (unlikely (bio_flagged (bio,BIO_CLONED )))return 0 ; -if (((bio->bi_iter.bi_size + len) >>9 ) > max_sectors) +if (((bio->bi_iter.bi_size + len) >>9 ) >queue_max_hw_sectors (q))return 0 ;/*
@@ -740,28 +754,7 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pageif (page == prev->bv_page && offset == prev->bv_offset + prev->bv_len) { -unsigned int prev_bv_len = prev->bv_len; prev->bv_len += len; - -if (q->merge_bvec_fn) { -struct bvec_merge_data bvm = { -/* prev_bvec is already charged in -bi_size, discharge it in order to -simulate merging updated prev_bvec -as new bvec. */ - .bi_bdev = bio->bi_bdev, - .bi_sector = bio->bi_iter.bi_sector, - .bi_size = bio->bi_iter.bi_size - - prev_bv_len, - .bi_rw = bio->bi_rw, - }; - -if (q->merge_bvec_fn (q, &bvm, prev) < prev->bv_len) { - prev->bv_len -= len; -return 0 ; - } - } - bio->bi_iter.bi_size += len; goto done; }
@@ -804,27 +797,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pageblk_recount_segments (q, bio); } -/* -* if queue has other restrictions (eg varying max sector size -* depending on offset), it can specify a merge_bvec_fn in the -* queue to get further control -*/ -if (q->merge_bvec_fn) { -struct bvec_merge_data bvm = { - .bi_bdev = bio->bi_bdev, - .bi_sector = bio->bi_iter.bi_sector, - .bi_size = bio->bi_iter.bi_size - len, - .bi_rw = bio->bi_rw, - }; - -/* -* merge_bvec_fn() returns number of bytes it can accept -* at this offset -*/ -if (q->merge_bvec_fn (q, &bvm, bvec) < bvec->bv_len) - goto failed; - } -/* If we may be able to merge these biovecs, force a recount */ if (bio->bi_vcnt >1 && (BIOVEC_PHYS_MERGEABLE (bvec-1 , bvec))) bio->bi_flags &= ~(1 <<BIO_SEG_VALID );
@@ -841,28 +813,6 @@ static int __bio_add_page(struct request_queue *q, struct bio *bio, struct pageblk_recount_segments (q, bio);return 0 ; } - -/** -* bio_add_pc_page - attempt to add page to bio -* @q: the target queue -* @bio: destination bio -* @page: page to add -* @len: vec entry length -* @offset: vec entry offset -* -* Attempt to add a page to the bio_vec maplist. This can fail for a -* number of reasons, such as the bio being full or target block device -* limitations. The target block device must allow bio's up to PAGE_SIZE, -* so it is always possible to add a single page to an empty bio. -* -* This should only be used by REQ_PC bios. -*/ -int bio_add_pc_page (struct request_queue *q,struct bio *bio,struct page *page, -unsigned int len,unsigned int offset) -{ -return __bio_add_page (q, bio, page, len, offset, -queue_max_hw_sectors (q)); -}EXPORT_SYMBOL (bio_add_pc_page);/**
@@ -872,22 +822,47 @@ EXPORT_SYMBOL(bio_add_pc_page);* @len: vec entry length * @offset: vec entry offset * -* Attempt to add a page to the bio_vec maplist. This can fail for a -* number of reasons, such as the bio being full or target block device -* limitations. The target block device must allow bio's up to PAGE_SIZE, -* so it is always possible to add a single page to an empty bio. +* Attempt to add a page to the bio_vec maplist. This will only fail +* if either bio->bi_vcnt == bio->bi_max_vecs or it's a cloned bio. */ -int bio_add_page (struct bio *bio,struct page *page,unsigned int len, -unsigned int offset) +int bio_add_page (struct bio *bio,struct page *page, +unsigned int len,unsigned int offset) { -struct request_queue *q =bdev_get_queue (bio->bi_bdev); -unsigned int max_sectors; +struct bio_vec *bv; - max_sectors =blk_max_size_offset (q, bio->bi_iter.bi_sector); -if ((max_sectors < (len >>9 )) && !bio->bi_iter.bi_size) - max_sectors = len >>9 ; +/* +* cloned bio must not modify vec list +*/ +if (WARN_ON_ONCE (bio_flagged (bio,BIO_CLONED ))) +return 0 ; -return __bio_add_page (q, bio, page, len, offset, max_sectors); +/* +* For filesystems with a blocksize smaller than the pagesize +* we will often be called with the same page as last time and +* a consecutive offset. Optimize this special case. +*/ +if (bio->bi_vcnt >0 ) { + bv = &bio->bi_io_vec[bio->bi_vcnt -1 ]; + +if (page == bv->bv_page && + offset == bv->bv_offset + bv->bv_len) { + bv->bv_len += len; + goto done; + } + } + +if (bio->bi_vcnt >= bio->bi_max_vecs) +return 0 ; + + bv = &bio->bi_io_vec[bio->bi_vcnt]; + bv->bv_page = page; + bv->bv_len = len; + bv->bv_offset = offset; + + bio->bi_vcnt++; +done: + bio->bi_iter.bi_size += len; +return len; }EXPORT_SYMBOL (bio_add_page);
Signature · 1 line
1.9.1
PATCH v5 3/11 bcache: remove driver private bio splitting code
In reply to Ming LinFrom: Kent Overstreet <[email protected]>
The bcache driver has always accepted arbitrarily large bios and split them internally. Now that every driver must accept arbitrarily large bios this code isn't nessecary anymore.
--- drivers/md/bcache/bcache.h | 18 -------- drivers/md/bcache/io.c | 100 +----------------------------------------- drivers/md/bcache/journal.c | 4 +- drivers/md/bcache/request.c | 16 +++---- drivers/md/bcache/super.c | 32 +------------- drivers/md/bcache/util.h | 5 ++- drivers/md/bcache/writeback.c | 4 +- 7 files changed, 18 insertions(+), 161 deletions(-)
drivers/md/bcache/bcache.h +0 -18
@@ -243,19 +243,6 @@ struct keybuf { DECLARE_ARRAY_ALLOCATOR(struct keybuf_key, freelist, KEYBUF_NR); }; -struct bio_split_pool { -struct bio_set *bio_split; -mempool_t *bio_split_hook; -}; - -struct bio_split_hook { -struct closure cl; -struct bio_split_pool *p; -struct bio *bio; -bio_end_io_t *bi_end_io; -void *bi_private; -}; -struct bcache_device {struct closure cl;
@@ -288,8 +275,6 @@ struct bcache_device {int (*cache_miss)(struct btree *,struct search *,struct bio *,unsigned );int (*ioctl) (struct bcache_device *,fmode_t ,unsigned ,unsigned long ); - -struct bio_split_pool bio_split_hook; };struct io {
@@ -454,8 +439,6 @@ struct cache {atomic_long_t meta_sectors_written;atomic_long_t btree_sectors_written;atomic_long_t sectors_written; - -struct bio_split_pool bio_split_hook; };struct gc_stat {
@@ -873,7 +856,6 @@ void bch_bbio_endio(struct cache_set *, struct bio *, int, const char *);void bch_bbio_free (struct bio *,struct cache_set *);struct bio *bch_bbio_alloc (struct cache_set *); -void bch_generic_make_request (struct bio *,struct bio_split_pool *);void __bch_submit_bbio (struct bio *,struct cache_set *);void bch_submit_bbio (struct bio *,struct cache_set *,struct bkey *,unsigned );
drivers/md/bcache/io.c +1 -99
112 lines not on this page · raw message
drivers/md/bcache/journal.c +2 -2
@@ -61,7 +61,7 @@ reread: left = ca->sb.bucket_size - offset; bio->bi_private = &cl;bch_bio_map (bio, data); -closure_bio_submit (bio, &cl, ca); +closure_bio_submit (bio, &cl);closure_sync (&cl);/* This function could be simpler now since we no longer write
@@ -646,7 +646,7 @@ static void journal_write_unlocked(struct closure *cl)spin_unlock (&c->journal.lock);while ((bio =bio_list_pop (&list))) -closure_bio_submit (bio, cl, c->cache[0 ]); +closure_bio_submit (bio, cl);continue_at (cl, journal_write_done,NULL ); }
drivers/md/bcache/request.c +8 -8
@@ -711,7 +711,7 @@ static void cached_dev_read_error(struct closure *cl)/* XXX: invalidate cache */ -closure_bio_submit (bio, cl, s->d); +closure_bio_submit (bio, cl); }continue_at (cl, cached_dev_cache_miss_done,NULL );
@@ -834,7 +834,7 @@ static int cached_dev_cache_miss(struct btree *b, struct search *s, s->cache_miss = miss; s->iop.bio = cache_bio;bio_get (cache_bio); -closure_bio_submit (cache_bio, &s->cl, s->d); +closure_bio_submit (cache_bio, &s->cl);return ret; out_put:
@@ -842,7 +842,7 @@ out_put: out_submit: miss->bi_end_io = request_endio; miss->bi_private = &s->cl; -closure_bio_submit (miss, &s->cl, s->d); +closure_bio_submit (miss, &s->cl);return ret; }
@@ -907,7 +907,7 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s)if (!(bio->bi_rw &REQ_DISCARD ) ||blk_queue_discard (bdev_get_queue (dc->bdev))) -closure_bio_submit (bio, cl, s->d); +closure_bio_submit (bio, cl); }else if (s->iop.writeback) {bch_writeback_add (dc); s->iop.bio = bio;
@@ -922,12 +922,12 @@ static void cached_dev_write(struct cached_dev *dc, struct search *s) flush->bi_end_io = request_endio; flush->bi_private = cl; -closure_bio_submit (flush, cl, s->d); +closure_bio_submit (flush, cl); } }else { s->iop.bio =bio_clone_fast (bio,GFP_NOIO , dc->disk.bio_split); -closure_bio_submit (bio, cl, s->d); +closure_bio_submit (bio, cl); }closure_call (&s->iop.cl, bch_data_insert,NULL , cl);
@@ -943,7 +943,7 @@ static void cached_dev_nodata(struct closure *cl)bch_journal_meta (s->iop.c, cl);/* If it's a flush, we send the flush to the backing device too */ -closure_bio_submit (bio, cl, s->d); +closure_bio_submit (bio, cl);continue_at (cl, cached_dev_bio_complete,NULL ); }
@@ -987,7 +987,7 @@ static void cached_dev_make_request(struct request_queue *q, struct bio *bio) !blk_queue_discard(bdev_get_queue(dc->bdev)))bio_endio (bio,0 ); else - bch_generic_make_request(bio, &d->bio_split_hook); +generic_make_request (bio ); } }
drivers/md/bcache/super.c +2 -30
@@ -59,29 +59,6 @@ struct workqueue_struct *bcache_wq;#define BTREE_MAX_PAGES (256 * 1024 / PAGE_SIZE) -static void bio_split_pool_free (struct bio_split_pool *p) -{ -if (p->bio_split_hook) -mempool_destroy (p->bio_split_hook); - -if (p->bio_split) -bioset_free (p->bio_split); -} - -static int bio_split_pool_init (struct bio_split_pool *p) -{ - p->bio_split =bioset_create (4 ,0 ); -if (!p->bio_split) -return -ENOMEM ; - - p->bio_split_hook =mempool_create_kmalloc_pool (4 , -sizeof (struct bio_split_hook )); -if (!p->bio_split_hook) -return -ENOMEM ; - -return 0 ; -} -/* Superblock */ static const char *read_super(struct cache_sb *sb, struct block_device *bdev,
@@ -537,7 +514,7 @@ static void prio_io(struct cache *ca, uint64_t bucket, unsigned long rw) bio->bi_private = ca;bch_bio_map (bio, ca->disk_buckets); -closure_bio_submit (bio, &ca->prio, ca); +closure_bio_submit (bio, &ca->prio);closure_sync (cl); }
@@ -757,7 +734,6 @@ static void bcache_device_free(struct bcache_device *d)put_disk (d->disk); } -bio_split_pool_free (&d->bio_split_hook);if (d->bio_split)bioset_free (d->bio_split);kvfree (d->full_dirty_stripes);
@@ -804,7 +780,6 @@ static int bcache_device_init(struct bcache_device *d, unsigned block_size,return minor;if (!(d->bio_split =bioset_create (4 , offsetof(struct bbio , bio))) || -bio_split_pool_init (&d->bio_split_hook) || !(d->disk =alloc_disk (1 ))) {ida_simple_remove (&bcache_minor, minor);return -ENOMEM ;
@@ -1793,8 +1768,6 @@ void bch_cache_release(struct kobject *kobj) ca->set->cache[ca->sb.nr_this_dev] =NULL ; } -bio_split_pool_free (&ca->bio_split_hook); -free_pages ((unsigned long ) ca->disk_buckets,ilog2 (bucket_pages (ca)));kfree (ca->prio_buckets);vfree (ca->buckets);
@@ -1839,8 +1812,7 @@ static int cache_alloc(struct cache_sb *sb, struct cache *ca) ca->sb.nbuckets)) || !(ca->prio_buckets =kzalloc (sizeof (uint64_t ) *prio_buckets (ca) *2 ,GFP_KERNEL )) || - !(ca->disk_buckets =alloc_bucket_pages (GFP_KERNEL , ca)) || - bio_split_pool_init(&ca->bio_split_hook)) + !(ca->disk_buckets = alloc_bucket_pages(GFP_KERNEL, ca))) return -ENOMEM; ca->prio_last_buckets = ca->prio_buckets +prio_buckets (ca);
drivers/md/bcache/util.h +3 -2
@@ -4,6 +4,7 @@#include <linux/blkdev.h> #include <linux/errno.h> +#include <linux/blkdev.h> #include <linux/kernel.h> #include <linux/llist.h> #include <linux/ratelimit.h>
@@ -570,10 +571,10 @@ static inline sector_t bdev_sectors(struct block_device *bdev)return bdev->bd_inode->i_size >>9 ; } -#define closure_bio_submit (bio, cl, dev) \ +#define closure_bio_submit (bio, cl) \ do { \ closure_get(cl); \ - bch_generic_make_request(bio, &(dev)->bio_split_hook); \ + generic_make_request(bio); \ } while (0)uint64_t bch_crc64_update (uint64_t ,const void *,size_t );
drivers/md/bcache/writeback.c +2 -2
@@ -188,7 +188,7 @@ static void write_dirty(struct closure *cl) io->bio.bi_bdev = io->dc->bdev; io->bio.bi_end_io = dirty_endio; -closure_bio_submit (&io->bio, cl, &io->dc->disk); +closure_bio_submit (&io->bio, cl);continue_at (cl, write_dirty_finish, system_wq); }
@@ -208,7 +208,7 @@ static void read_dirty_submit(struct closure *cl) { struct dirty_io *io = container_of(cl, struct dirty_io, cl); -closure_bio_submit (&io->bio, cl, &io->dc->disk); +closure_bio_submit (&io->bio, cl);continue_at (cl, write_dirty, system_wq); }
Signature · 1 line
1.9.1
PATCH v5 4/11 btrfs: remove bio splitting and merge_bvec_fn() calls
In reply to Ming LinFrom: Kent Overstreet <[email protected]>
Btrfs has been doing bio splitting from btrfs_map_bio(), by checking device limits as well as calling ->merge_bvec_fn() etc. That is not necessary any more, because generic_make_request() is now able to handle arbitrarily sized bios. So clean up unnecessary code paths.
--- fs/btrfs/volumes.c | 72 ------------------------------------------------------ 1 file changed, 72 deletions(-)
fs/btrfs/volumes.c +0 -72
@@ -5833,34 +5833,6 @@ static noinline void btrfs_schedule_bio(struct btrfs_root *root, &device->work); } -static int bio_size_ok (struct block_device *bdev,struct bio *bio, -sector_t sector) -{ -struct bio_vec *prev; -struct request_queue *q =bdev_get_queue (bdev); -unsigned int max_sectors =queue_max_sectors (q); -struct bvec_merge_data bvm = { - .bi_bdev = bdev, - .bi_sector = sector, - .bi_rw = bio->bi_rw, - }; - -if (WARN_ON (bio->bi_vcnt ==0 )) -return 1 ; - - prev = &bio->bi_io_vec[bio->bi_vcnt -1 ]; -if (bio_sectors (bio) > max_sectors) -return 0 ; - -if (!q->merge_bvec_fn) -return 1 ; - - bvm.bi_size = bio->bi_iter.bi_size - prev->bv_len; -if (q->merge_bvec_fn (q, &bvm, prev) < prev->bv_len) -return 0 ; -return 1 ; -} -static void submit_stripe_bio (struct btrfs_root *root,struct btrfs_bio *bbio,struct bio *bio,u64 physical,int dev_nr,int rw,int async)
@@ -5894,38 +5866,6 @@ static void submit_stripe_bio(struct btrfs_root *root, struct btrfs_bio *bbio,btrfsic_submit_bio (rw, bio); } -static int breakup_stripe_bio (struct btrfs_root *root,struct btrfs_bio *bbio, -struct bio *first_bio,struct btrfs_device *dev, -int dev_nr,int rw,int async) -{ -struct bio_vec *bvec = first_bio->bi_io_vec; -struct bio *bio; -int nr_vecs =bio_get_nr_vecs (dev->bdev); -u64 physical = bbio->stripes[dev_nr].physical; - -again: - bio =btrfs_bio_alloc (dev->bdev, physical >>9 , nr_vecs,GFP_NOFS ); -if (!bio) -return -ENOMEM ; - -while (bvec <= (first_bio->bi_io_vec + first_bio->bi_vcnt -1 )) { -if (bio_add_page (bio, bvec->bv_page, bvec->bv_len, - bvec->bv_offset) < bvec->bv_len) { -u64 len = bio->bi_iter.bi_size; - -atomic_inc (&bbio->stripes_pending); -submit_stripe_bio (root, bbio, bio, physical, dev_nr, - rw, async); - physical += len; - goto again; - } - bvec++; - } - -submit_stripe_bio (root, bbio, bio, physical, dev_nr, rw, async); -return 0 ; -} -static void bbio_error (struct btrfs_bio *bbio,struct bio *bio,u64 logical) {atomic_inc (&bbio->error);
@@ -5998,18 +5938,6 @@ int btrfs_map_bio(struct btrfs_root *root, int rw, struct bio *bio,continue ; } -/* -* Check and see if we're ok with this bio based on it's size -* and offset with the given device. -*/ -if (!bio_size_ok (dev->bdev, first_bio, - bbio->stripes[dev_nr].physical >>9 )) { - ret =breakup_stripe_bio (root, bbio, first_bio, dev, - dev_nr, rw, async_submit); -BUG_ON (ret); -continue ; - } -if (dev_nr < total_devs -1 ) { bio =btrfs_bio_clone (first_bio,GFP_NOFS );BUG_ON (!bio);/* -ENOMEM */
Signature · 1 line
1.9.1
PATCH v5 5/11 block: remove split code in blkdev_issue_discard
In reply to Ming LinFrom: Ming Lin <[email protected]>
The split code in blkdev_issue_discard() can go away now that any driver that cares does the split.
--- block/blk-lib.c | 73 +++++++++++---------------------------------------------- 1 file changed, 14 insertions(+), 59 deletions(-)
block/blk-lib.c +14 -59
@@ -43,34 +43,17 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,DECLARE_COMPLETION_ONSTACK (wait);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 ;struct blk_plug plug; -if (!q) +if (!q || !nr_sects)return -ENXIO ;if (!blk_queue_discard (q))return -EOPNOTSUPP ; -/* 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 ); - 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))return -EOPNOTSUPP ;
@@ -82,52 +65,24 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector, bb.wait = &wait;blk_start_plug (&plug); -while (nr_sects) { -unsigned int req_sects; -sector_t end_sect, tmp; - bio =bio_alloc (gfp_mask,1 ); -if (!bio) { - ret = -ENOMEM ; -break ; - } + bio =bio_alloc (gfp_mask,1 ); +if (!bio) { + ret = -ENOMEM ; + goto out; + } - 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; + bio->bi_bdev = bdev; + bio->bi_private = &bb; - bio->bi_iter.bi_sector = sector; - bio->bi_end_io = bio_batch_end_io; - bio->bi_bdev = bdev; - bio->bi_private = &bb; + bio->bi_iter.bi_size = nr_sects <<9 ; - bio->bi_iter.bi_size = req_sects <<9 ; - nr_sects -= req_sects; - sector = end_sect; +atomic_inc (&bb.done); +submit_bio (type, bio); -atomic_inc (&bb.done); -submit_bio (type, bio); - -/* -* We can loop for a long time in here, if someone does -* full device discards (like mkfs). Be nice and allow -* us to schedule out to avoid softlocking if preempt -* is disabled. -*/ -cond_resched (); - } +out:blk_finish_plug (&plug);/* Wait for bios in-flight */
Signature · 1 line
1.9.1
PATCH v5 6/11 md/raid5: split bio for chunk_aligned_read
In reply to Ming LinFrom: Ming Lin <[email protected]>
If a read request fits entirely in a chunk, it will be passed directly to the underlying device (providing it hasn't failed of course). If it doesn't fit, the slightly less efficient path that uses the stripe_cache is used. Requests that get to the stripe cache are always completely split up as necessary.
So with RAID5, ripping out the merge_bvec_fn doesn't cause it to stop work, but could cause it to take the less efficient path more often.
All that is needed to manage this is for 'chunk_aligned_read' do some bio splitting, much like the RAID0 code does.
--- drivers/md/raid5.c | 37 ++++++++++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 5 deletions(-)
drivers/md/raid5.c +32 -5
@@ -4799,7 +4799,7 @@ static int bio_fits_rdev(struct bio *bi)return 1 ; } -static int chunk_aligned_read (struct mddev *mddev,struct bio * raid_bio) +static int raid5_read_one_chunk (struct mddev *mddev,struct bio *raid_bio) {struct r5conf *conf = mddev->private;int dd_idx;
@@ -4808,7 +4808,7 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio)sector_t end_sector;if (!in_chunk_boundary (mddev, raid_bio)) { -pr_debug ("chunk_aligned_read : non aligned\n" ); +pr_debug ("%s: non aligned\n" , __func__);return 0 ; }/*
@@ -4885,6 +4885,31 @@ static int chunk_aligned_read(struct mddev *mddev, struct bio * raid_bio) } } +static struct bio *chunk_aligned_read (struct mddev *mddev,struct bio *raid_bio) +{ +struct bio *split; + +do { +sector_t sector = raid_bio->bi_iter.bi_sector; +unsigned chunk_sects = mddev->chunk_sectors; +unsigned sectors = chunk_sects - (sector & (chunk_sects-1 )); + +if (sectors <bio_sectors (raid_bio)) { + split =bio_split (raid_bio, sectors,GFP_NOIO , fs_bio_set); +bio_chain (split, raid_bio); + }else + split = raid_bio; + +if (!raid5_read_one_chunk (mddev, split)) { +if (split != raid_bio) +generic_make_request (raid_bio); +return split; + } + }while (split != raid_bio); + +return NULL ; +} +/* __get_priority_stripe - get the next stripe to process * * Full stripe writes are allowed to pass preread active stripes up until
@@ -5162,9 +5187,11 @@ static void make_request(struct mddev *mddev, struct bio * bi)* data on failed drives. */ if (rw ==READ && mddev->degraded ==0 && - mddev->reshape_position == MaxSector && -chunk_aligned_read (mddev,bi)) -return ; + mddev->reshape_position == MaxSector) { + bi =chunk_aligned_read (mddev, bi); +if (!bi) +return ; + }if (unlikely (bi->bi_rw &REQ_DISCARD )) {make_discard_request (mddev, bi);
Signature · 1 line
1.9.1
PATCH v5 7/11 md/raid5: get rid of bio_fits_rdev()
In reply to Ming LinFrom: Kent Overstreet <[email protected]>
Remove bio_fits_rdev() as sufficient merge_bvec_fn() handling is now performed by blk_queue_split() in md_make_request().
--- drivers/md/raid5.c | 23 +---------------------- 1 file changed, 1 insertion(+), 22 deletions(-)
drivers/md/raid5.c +1 -22
@@ -4780,25 +4780,6 @@ static void raid5_align_endio(struct bio *bi, int error)add_bio_to_retry (raid_bi, conf); } -static int bio_fits_rdev (struct bio *bi) -{ -struct request_queue *q =bdev_get_queue (bi->bi_bdev); - -if (bio_sectors (bi) >queue_max_sectors (q)) -return 0 ; -blk_recount_segments (q, bi); -if (bi->bi_phys_segments >queue_max_segments (q)) -return 0 ; - -if (q->merge_bvec_fn) -/* it's too hard to apply the merge_bvec_fn at this stage, -* just just give up -*/ -return 0 ; - -return 1 ; -} -static int raid5_read_one_chunk (struct mddev *mddev,struct bio *raid_bio) {struct r5conf *conf = mddev->private;
@@ -4852,11 +4833,9 @@ static int raid5_read_one_chunk(struct mddev *mddev, struct bio *raid_bio) align_bi->bi_bdev = rdev->bdev;__clear_bit (BIO_SEG_VALID , &align_bi->bi_flags); -if (!bio_fits_rdev (align_bi) || -is_badblock (rdev, align_bi->bi_iter.bi_sector, +if (is_badblock (rdev, align_bi->bi_iter.bi_sector,bio_sectors (align_bi), &first_bad, &bad_sectors)) { -/* too big in some way, or has a known bad block */ bio_put (align_bi);rdev_dec_pending (rdev, mddev);return 0 ;
Signature · 1 line
1.9.1
PATCH v5 8/11 block: kill merge_bvec_fn() completely
In reply to Ming LinFrom: Kent Overstreet <[email protected]>
As generic_make_request() is now able to handle arbitrarily sized bios, it's no longer necessary for each individual block driver to define its own ->merge_bvec_fn() callback. Remove every invocation completely.
[dpark: also remove ->merge_bvec_fn() in dm-thin as well as
dm-era-target, and resolve merge conflicts]
--- block/blk-merge.c | 17 +----- block/blk-settings.c | 22 ------- drivers/block/drbd/drbd_int.h | 1 - drivers/block/drbd/drbd_main.c | 1 - drivers/block/drbd/drbd_req.c | 35 ------------ drivers/block/pktcdvd.c | 21 ------- drivers/block/rbd.c | 47 --------------- drivers/md/dm-cache-target.c | 21 ------- drivers/md/dm-crypt.c | 16 ------ drivers/md/dm-era-target.c | 15 ----- drivers/md/dm-flakey.c | 16 ------ drivers/md/dm-linear.c | 16 ------ drivers/md/dm-log-writes.c | 16 ------ drivers/md/dm-raid.c | 19 ------ drivers/md/dm-snap.c | 15 ----- drivers/md/dm-stripe.c | 21 ------- drivers/md/dm-table.c | 8 --- drivers/md/dm-thin.c | 31 ---------- drivers/md/dm-verity.c | 16 ------ drivers/md/dm.c | 127 +---------------------------------------- drivers/md/dm.h | 2 - drivers/md/linear.c | 43 -------------- drivers/md/md.c | 26 --------- drivers/md/md.h | 12 ---- drivers/md/multipath.c | 21 ------- drivers/md/raid0.c | 56 ------------------ drivers/md/raid0.h | 2 - drivers/md/raid1.c | 58 +------------------ drivers/md/raid10.c | 121 +-------------------------------------- drivers/md/raid5.c | 32 ----------- include/linux/blkdev.h | 10 ---- include/linux/device-mapper.h | 4 -- 32 files changed, 9 insertions(+), 859 deletions(-)
block/blk-merge.c +3 -14
@@ -69,24 +69,13 @@ static struct bio *blk_bio_segment_split(struct request_queue *q,struct bio *split;struct bio_vec bv, bvprv;struct bvec_iter iter; -unsigned seg_size =0 , nsegs =0 ; +unsigned seg_size =0 , nsegs =0 , sectors =0 ;int prev =0 ; -struct bvec_merge_data bvm = { - .bi_bdev = bio->bi_bdev, - .bi_sector = bio->bi_iter.bi_sector, - .bi_size =0 , - .bi_rw = bio->bi_rw, - }; -bio_for_each_segment (bv, bio, iter) { -if (q->merge_bvec_fn && - q->merge_bvec_fn (q, &bvm, &bv) < (int ) bv.bv_len) - goto split; - - bvm.bi_size += bv.bv_len; + sectors += bv.bv_len >>9 ; -if (bvm.bi_size >>9 >queue_max_sectors (q)) +if (sectors >queue_max_sectors (q)) goto split;/*
block/blk-settings.c +0 -22
@@ -53,28 +53,6 @@ void blk_queue_unprep_rq(struct request_queue *q, unprep_rq_fn *ufn) }EXPORT_SYMBOL (blk_queue_unprep_rq); -/** -* blk_queue_merge_bvec - set a merge_bvec function for queue -* @q: queue -* @mbfn: merge_bvec_fn -* -* Usually queues have static limitations on the max sectors or segments that -* we can put in a request. Stacking drivers may have some settings that -* are dynamic, and thus we have to query the queue whether it is ok to -* add a new bio_vec to a bio at a given offset or not. If the block device -* has such limitations, it needs to register a merge_bvec_fn to control -* the size of bio's sent to it. Note that a block device *must* allow a -* single page to be added to an empty bio. The block device driver may want -* to use the bio_split() function to deal with these bio's. By default -* no merge_bvec_fn is defined for a queue, and only the fixed limits are -* honored. -*/ -void blk_queue_merge_bvec (struct request_queue *q,merge_bvec_fn *mbfn) -{ - q->merge_bvec_fn = mbfn; -} -EXPORT_SYMBOL (blk_queue_merge_bvec); -void blk_queue_softirq_done (struct request_queue *q,softirq_done_fn *fn) { q->softirq_done_fn = fn;
drivers/block/drbd/drbd_int.h +0 -1
@@ -1450,7 +1450,6 @@ extern void do_submit(struct work_struct *ws);extern void __drbd_make_request (struct drbd_device *,struct bio *,unsigned long );extern void drbd_make_request (struct request_queue *q,struct bio *bio);extern int drbd_read_remote (struct drbd_device *device,struct drbd_request *req); -extern int drbd_merge_bvec (struct request_queue *q,struct bvec_merge_data *bvm,struct bio_vec *bvec);extern int is_valid_ar_handle (struct drbd_request *,sector_t );
drivers/block/drbd/drbd_main.c +0 -1
@@ -2774,7 +2774,6 @@ enum drbd_ret_code drbd_create_device(struct drbd_config_context *adm_ctx, unsigThis triggers a max_bio_size message upon first attach or connect */ blk_queue_max_hw_sectors (q,DRBD_MAX_BIO_SIZE_SAFE >>8 );blk_queue_bounce_limit (q,BLK_BOUNCE_ANY ); -blk_queue_merge_bvec (q, drbd_merge_bvec); q->queue_lock = &resource->req_lock; device->md_io.page =alloc_page (GFP_KERNEL );
drivers/block/drbd/drbd_req.c +0 -35
@@ -1510,41 +1510,6 @@ void drbd_make_request(struct request_queue *q, struct bio *bio)__drbd_make_request (device, bio, start_jif); } -/* This is called by bio_add_page(). -* -* q->max_hw_sectors and other global limits are already enforced there. -* -* We need to call down to our lower level device, -* in case it has special restrictions. -* -* We also may need to enforce configured max-bio-bvecs limits. -* -* As long as the BIO is empty we have to allow at least one bvec, -* regardless of size and offset, so no need to ask lower levels. -*/ -int drbd_merge_bvec (struct request_queue *q,struct bvec_merge_data *bvm,struct bio_vec *bvec) -{ -struct drbd_device *device = (struct drbd_device *) q->queuedata; -unsigned int bio_size = bvm->bi_size; -int limit =DRBD_MAX_BIO_SIZE ; -int backing_limit; - -if (bio_size &&get_ldev (device)) { -unsigned int max_hw_sectors =queue_max_hw_sectors (q); -struct request_queue *const b = - device->ldev->backing_bdev->bd_disk->queue; -if (b->merge_bvec_fn) { - bvm->bi_bdev = device->ldev->backing_bdev; - backing_limit = b->merge_bvec_fn (b, bvm, bvec); - limit =min (limit, backing_limit); - } -put_ldev (device); -if ((limit >>9 ) > max_hw_sectors) - limit = max_hw_sectors <<9 ; - } -return limit; -} -void request_timer_fn (unsigned long data) {struct drbd_device *device = (struct drbd_device *) data;
drivers/block/pktcdvd.c +0 -21
@@ -2506,26 +2506,6 @@ end_io: -static int pkt_merge_bvec (struct request_queue *q,struct bvec_merge_data *bmd, -struct bio_vec *bvec) -{ -struct pktcdvd_device *pd = q->queuedata; -sector_t zone =get_zone (bmd->bi_sector, pd); -int used = ((bmd->bi_sector - zone) <<9 ) + bmd->bi_size; -int remaining = (pd->settings.size <<9 ) - used; -int remaining2; - -/* -* A bio <= PAGE_SIZE must be allowed. If it crosses a packet -* boundary, pkt_make_request() will split the bio. -*/ - remaining2 =PAGE_SIZE - bmd->bi_size; - remaining =max (remaining, remaining2); - -BUG_ON (remaining <0 ); -return remaining; -} -static void pkt_init_queue (struct pktcdvd_device *pd) {struct request_queue *q = pd->disk->queue;
@@ -2533,7 +2513,6 @@ static void pkt_init_queue(struct pktcdvd_device *pd)blk_queue_make_request (q, pkt_make_request);blk_queue_logical_block_size (q,CD_FRAMESIZE );blk_queue_max_hw_sectors (q,PACKET_MAX_SECTORS ); -blk_queue_merge_bvec (q, pkt_merge_bvec); q->queuedata = pd; }
drivers/block/rbd.c +0 -47
59 lines not on this page · raw message
drivers/md/dm-cache-target.c +0 -21
@@ -3769,26 +3769,6 @@ static int cache_iterate_devices(struct dm_target *ti,return r; } -/* -* We assume I/O is going to the origin (which is the volume -* more likely to have restrictions e.g. by being striped). -* (Looking up the exact location of the data would be expensive -* and could always be out of date by the time the bio is submitted.) -*/ -static int cache_bvec_merge (struct dm_target *ti, -struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct cache *cache = ti->private; -struct request_queue *q =bdev_get_queue (cache->origin_dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = cache->origin_dev->bdev; -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static void set_discard_limits (struct cache *cache,struct queue_limits *limits) {/*
@@ -3832,7 +3812,6 @@ static struct target_type cache_target = { .status = cache_status, .message = cache_message, .iterate_devices = cache_iterate_devices, - .merge = cache_bvec_merge, .io_hints = cache_io_hints, };
drivers/md/dm-crypt.c +0 -16
@@ -2035,21 +2035,6 @@ error:return -EINVAL ; } -static int crypt_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct crypt_config *cc = ti->private; -struct request_queue *q =bdev_get_queue (cc->dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = cc->dev->bdev; - bvm->bi_sector = cc->start +dm_target_offset (ti, bvm->bi_sector); - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static int crypt_iterate_devices (struct dm_target *ti,iterate_devices_callout_fn fn,void *data) {
@@ -2070,7 +2055,6 @@ static struct target_type crypt_target = { .preresume = crypt_preresume, .resume = crypt_resume, .message = crypt_message, - .merge = crypt_merge, .iterate_devices = crypt_iterate_devices, };
drivers/md/dm-era-target.c +0 -15
@@ -1673,20 +1673,6 @@ static int era_iterate_devices(struct dm_target *ti,return fn (ti, era->origin_dev,0 ,get_dev_size (era->origin_dev), data); } -static int era_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct era *era = ti->private; -struct request_queue *q =bdev_get_queue (era->origin_dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = era->origin_dev->bdev; - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static void era_io_hints (struct dm_target *ti,struct queue_limits *limits) {struct era *era = ti->private;
@@ -1717,7 +1703,6 @@ static struct target_type era_target = { .status = era_status, .message = era_message, .iterate_devices = era_iterate_devices, - .merge = era_merge, .io_hints = era_io_hints };
drivers/md/dm-flakey.c +0 -16
@@ -387,21 +387,6 @@ static int flakey_ioctl(struct dm_target *ti, unsigned int cmd, unsigned long arreturn r ? :__blkdev_driver_ioctl (dev->bdev, dev->mode, cmd, arg); } -static int flakey_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct flakey_c *fc = ti->private; -struct request_queue *q =bdev_get_queue (fc->dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = fc->dev->bdev; - bvm->bi_sector =flakey_map_sector (ti, bvm->bi_sector); - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static int flakey_iterate_devices (struct dm_target *ti,iterate_devices_callout_fn fn,void *data) {struct flakey_c *fc = ti->private;
@@ -419,7 +404,6 @@ static struct target_type flakey_target = { .end_io = flakey_end_io, .status = flakey_status, .ioctl = flakey_ioctl, - .merge = flakey_merge, .iterate_devices = flakey_iterate_devices, };
drivers/md/dm-linear.c +0 -16
@@ -130,21 +130,6 @@ static int linear_ioctl(struct dm_target *ti, unsigned int cmd,return r ? :__blkdev_driver_ioctl (dev->bdev, dev->mode, cmd, arg); } -static int linear_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct linear_c *lc = ti->private; -struct request_queue *q =bdev_get_queue (lc->dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = lc->dev->bdev; - bvm->bi_sector =linear_map_sector (ti, bvm->bi_sector); - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static int linear_iterate_devices (struct dm_target *ti,iterate_devices_callout_fn fn,void *data) {
@@ -162,7 +147,6 @@ static struct target_type linear_target = { .map = linear_map, .status = linear_status, .ioctl = linear_ioctl, - .merge = linear_merge, .iterate_devices = linear_iterate_devices, };
drivers/md/dm-log-writes.c +0 -16
@@ -728,21 +728,6 @@ static int log_writes_ioctl(struct dm_target *ti, unsigned int cmd,return r ? :__blkdev_driver_ioctl (dev->bdev, dev->mode, cmd, arg); } -static int log_writes_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct log_writes_c *lc = ti->private; -struct request_queue *q =bdev_get_queue (lc->dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = lc->dev->bdev; - bvm->bi_sector =dm_target_offset (ti, bvm->bi_sector); - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static int log_writes_iterate_devices (struct dm_target *ti,iterate_devices_callout_fn fn,void *data)
@@ -796,7 +781,6 @@ static struct target_type log_writes_target = { .end_io = normal_end_io, .status = log_writes_status, .ioctl = log_writes_ioctl, - .merge = log_writes_merge, .message = log_writes_message, .iterate_devices = log_writes_iterate_devices, .io_hints = log_writes_io_hints,
drivers/md/dm-raid.c +0 -19
@@ -1717,24 +1717,6 @@ static void raid_resume(struct dm_target *ti)mddev_resume (&rs->md); } -static int raid_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct raid_set *rs = ti->private; -struct md_personality *pers = rs->md.pers; - -if (pers && pers->mergeable_bvec) -return min (max_size, pers->mergeable_bvec (&rs->md, bvm, biovec)); - -/* -* In case we can't request the personality because -* the raid set is not running yet -* -* -> return safe minimum -*/ -return rs->md.chunk_sectors; -} -static struct target_type raid_target = { .name ="raid" , .version = {1 ,7 ,0 },
@@ -1749,7 +1731,6 @@ static struct target_type raid_target = { .presuspend = raid_presuspend, .postsuspend = raid_postsuspend, .resume = raid_resume, - .merge = raid_merge, };static int __init dm_raid_init (void )
drivers/md/dm-snap.c +0 -15
@@ -2330,20 +2330,6 @@ static void origin_status(struct dm_target *ti, status_type_t type, } } -static int origin_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct dm_origin *o = ti->private; -struct request_queue *q =bdev_get_queue (o->dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = o->dev->bdev; - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static int origin_iterate_devices (struct dm_target *ti,iterate_devices_callout_fn fn,void *data) {
@@ -2362,7 +2348,6 @@ static struct target_type origin_target = { .resume = origin_resume, .postsuspend = origin_postsuspend, .status = origin_status, - .merge = origin_merge, .iterate_devices = origin_iterate_devices, };
drivers/md/dm-stripe.c +0 -21
@@ -412,26 +412,6 @@ static void stripe_io_hints(struct dm_target *ti,blk_limits_io_opt (limits, chunk_size * sc->stripes); } -static int stripe_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct stripe_c *sc = ti->private; -sector_t bvm_sector = bvm->bi_sector; -uint32_t stripe; -struct request_queue *q; - -stripe_map_sector (sc, bvm_sector, &stripe, &bvm_sector); - - q =bdev_get_queue (sc->stripe[stripe].dev->bdev); -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = sc->stripe[stripe].dev->bdev; - bvm->bi_sector = sc->stripe[stripe].physical_start + bvm_sector; - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static struct target_type stripe_target = { .name ="striped" , .version = {1 ,5 ,1 },
@@ -443,7 +423,6 @@ static struct target_type stripe_target = { .status = stripe_status, .iterate_devices = stripe_iterate_devices, .io_hints = stripe_io_hints, - .merge = stripe_merge, };int __init dm_stripe_init (void )
drivers/md/dm-table.c +0 -8
@@ -440,14 +440,6 @@ static int dm_set_device_limits(struct dm_target *ti, struct dm_dev *dev, q->limits.alignment_offset, (unsigned long long) start << SECTOR_SHIFT); -/* -* Check if merge fn is supported. -* If not we'll force DM to use PAGE_SIZE or -* smaller I/O, just to be safe. -*/ -if (dm_queue_merge_is_compulsory (q) && !ti->type->merge) -blk_limits_max_hw_sectors (limits, - (unsigned int ) (PAGE_SIZE >>9 ));return 0 ; }
drivers/md/dm-thin.c +0 -31
@@ -3840,20 +3840,6 @@ static int pool_iterate_devices(struct dm_target *ti,return fn (ti, pt->data_dev,0 , ti->len, data); } -static int pool_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct pool_c *pt = ti->private; -struct request_queue *q =bdev_get_queue (pt->data_dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = pt->data_dev->bdev; - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static void pool_io_hints (struct dm_target *ti,struct queue_limits *limits) {struct pool_c *pt = ti->private;
@@ -3930,7 +3916,6 @@ static struct target_type pool_target = { .resume = pool_resume, .message = pool_message, .status = pool_status, - .merge = pool_merge, .iterate_devices = pool_iterate_devices, .io_hints = pool_io_hints, };
@@ -4257,21 +4242,6 @@ err:DMEMIT ("Error" ); } -static int thin_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct thin_c *tc = ti->private; -struct request_queue *q =bdev_get_queue (tc->pool_dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = tc->pool_dev->bdev; - bvm->bi_sector =dm_target_offset (ti, bvm->bi_sector); - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static int thin_iterate_devices (struct dm_target *ti,iterate_devices_callout_fn fn,void *data) {
@@ -4315,7 +4285,6 @@ static struct target_type thin_target = { .presuspend = thin_presuspend, .postsuspend = thin_postsuspend, .status = thin_status, - .merge = thin_merge, .iterate_devices = thin_iterate_devices, .io_hints = thin_io_hints, };
drivers/md/dm-verity.c +0 -16
@@ -648,21 +648,6 @@ static int verity_ioctl(struct dm_target *ti, unsigned cmd, cmd, arg); } -static int verity_merge (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size) -{ -struct dm_verity *v = ti->private; -struct request_queue *q =bdev_get_queue (v->data_dev->bdev); - -if (!q->merge_bvec_fn) -return max_size; - - bvm->bi_bdev = v->data_dev->bdev; - bvm->bi_sector =verity_map_sector (v, bvm->bi_sector); - -return min (max_size, q->merge_bvec_fn (q, bvm, biovec)); -} -static int verity_iterate_devices (struct dm_target *ti,iterate_devices_callout_fn fn,void *data) {
@@ -995,7 +980,6 @@ static struct target_type verity_target = { .map = verity_map, .status = verity_status, .ioctl = verity_ioctl, - .merge = verity_merge, .iterate_devices = verity_iterate_devices, .io_hints = verity_io_hints, };
drivers/md/dm.c +2 -125
168 lines not on this page · raw message
drivers/md/dm.h +0 -2
@@ -78,8 +78,6 @@ bool dm_table_mq_request_based(struct dm_table *t);void dm_table_free_md_mempools (struct dm_table *t);struct dm_md_mempools *dm_table_get_md_mempools (struct dm_table *t); -int dm_queue_merge_is_compulsory (struct request_queue *q); -void dm_lock_md_type (struct mapped_device *md);void dm_unlock_md_type (struct mapped_device *md);void dm_set_md_type (struct mapped_device *md,unsigned type);
drivers/md/linear.c +0 -43
55 lines not on this page · raw message
drivers/md/md.c +0 -26
@@ -352,29 +352,6 @@ static int md_congested(void *data, int bits)return mddev_congested (mddev, bits); } -static int md_mergeable_bvec (struct request_queue *q, -struct bvec_merge_data *bvm, -struct bio_vec *biovec) -{ -struct mddev *mddev = q->queuedata; -int ret; -rcu_read_lock (); -if (mddev->suspended) { -/* Must always allow one vec */ -if (bvm->bi_size ==0 ) - ret = biovec->bv_len; -else - ret =0 ; - }else { -struct md_personality *pers = mddev->pers; -if (pers && pers->mergeable_bvec) - ret = pers->mergeable_bvec (mddev, bvm, biovec); -else - ret = biovec->bv_len; - } -rcu_read_unlock (); -return ret; -}/* * Generic flush handling for md */
@@ -5188,7 +5165,6 @@ int md_run(struct mddev *mddev)if (mddev->queue) { mddev->queue->backing_dev_info.congested_data = mddev; mddev->queue->backing_dev_info.congested_fn = md_congested; -blk_queue_merge_bvec (mddev->queue, md_mergeable_bvec); }if (pers->sync_request) { if (mddev->kobj.sd &&
@@ -5317,7 +5293,6 @@ static void md_clean(struct mddev *mddev) mddev->degraded =0 ; mddev->safemode =0 ; mddev->private =NULL ; - mddev->merge_check_needed =0 ; mddev->bitmap_info.offset =0 ; mddev->bitmap_info.default_offset =0 ; mddev->bitmap_info.default_space =0 ;
@@ -5514,7 +5489,6 @@ static int do_md_stop(struct mddev *mddev, int mode,__md_stop_writes (mddev);__md_stop (mddev); - mddev->queue->merge_bvec_fn =NULL ; mddev->queue->backing_dev_info.congested_fn =NULL ;/* tell userspace to handle 'inactive' */
drivers/md/md.h +0 -12
@@ -134,10 +134,6 @@ enum flag_bits { Bitmap_sync,/* ..actually, not quite In_sync. Need a * bitmap-based recovery to get fully in sync */ - Unmerged,/* device is being added to array and should -* be considerred for bvec_merge_fn but not -* yet for actual IO -*/ WriteMostly,/* Avoid reading if at all possible */ AutoDetected,/* added by auto-detect */ Blocked, /* An error occurred but has not yet
@@ -374,10 +370,6 @@ struct mddev {int degraded;/* whether md should consider * adding a spare */ -int merge_check_needed;/* at least one -* member device -* has a -* merge_bvec_fn */ atomic_t recovery_active;/* blocks scheduled, but not written */ wait_queue_head_t recovery_wait;
@@ -532,10 +524,6 @@ struct md_personality/* congested implements bdi.congested_fn(). * Will not be called while array is 'suspended' */ int (*congested)(struct mddev *mddev,int bits); -/* mergeable_bvec is use to implement ->merge_bvec_fn */ -int (*mergeable_bvec)(struct mddev *mddev, -struct bvec_merge_data *bvm, -struct bio_vec *biovec); };struct md_sysfs_entry {
drivers/md/multipath.c +0 -21
@@ -257,18 +257,6 @@ static int multipath_add_disk(struct mddev *mddev, struct md_rdev *rdev)disk_stack_limits (mddev->gendisk, rdev->bdev, rdev->data_offset <<9 ); -/* as we don't honour merge_bvec_fn, we must never risk -* violating it, so limit ->max_segments to one, lying -* within a single page. -* (Note: it is very unlikely that a device with -* merge_bvec_fn will be involved in multipath.) -*/ -if (q->merge_bvec_fn) { -blk_queue_max_segments (mddev->queue,1 ); -blk_queue_segment_boundary (mddev->queue, -PAGE_CACHE_SIZE -1 ); - } -spin_lock_irq (&conf->device_lock); mddev->degraded--; rdev->raid_disk = path;
@@ -432,15 +420,6 @@ static int multipath_run (struct mddev *mddev)disk_stack_limits (mddev->gendisk, rdev->bdev, rdev->data_offset <<9 ); -/* as we don't honour merge_bvec_fn, we must never risk -* violating it, not that we ever expect a device with -* a merge_bvec_fn to be involved in multipath */ -if (rdev->bdev->bd_disk->queue->merge_bvec_fn) { -blk_queue_max_segments (mddev->queue,1 ); -blk_queue_segment_boundary (mddev->queue, -PAGE_CACHE_SIZE -1 ); - } -if (!test_bit (Faulty, &rdev->flags)) working_disks++; }
drivers/md/raid0.c +0 -56
74 lines not on this page · raw message
drivers/md/raid0.h +0 -2
@@ -12,8 +12,6 @@ struct r0conf {struct md_rdev **devlist;/* lists of rdevs, pointed to * by strip_zone->dev */ int nr_strip_zones; -int has_merge_bvec;/* at least one member has - * a merge_bvec_fn */ };#endif
drivers/md/raid1.c +1 -57
106 lines not on this page · raw message
drivers/md/raid10.c +3 -118
185 lines not on this page · raw message
drivers/md/raid5.c +0 -32
@@ -4662,35 +4662,6 @@ static int raid5_congested(struct mddev *mddev, int bits)return 0 ; } -/* We want read requests to align with chunks where possible, -* but write requests don't need to. -*/ -static int raid5_mergeable_bvec (struct mddev *mddev, -struct bvec_merge_data *bvm, -struct bio_vec *biovec) -{ -sector_t sector = bvm->bi_sector +get_start_sect (bvm->bi_bdev); -int max; -unsigned int chunk_sectors = mddev->chunk_sectors; -unsigned int bio_sectors = bvm->bi_size >>9 ; - -/* -* always allow writes to be mergeable, read as well if array -* is degraded as we'll go through stripe cache anyway. -*/ -if ((bvm->bi_rw &1 ) ==WRITE || mddev->degraded) -return biovec->bv_len; - -if (mddev->new_chunk_sectors < mddev->chunk_sectors) - chunk_sectors = mddev->new_chunk_sectors; - max = (chunk_sectors - ((sector & (chunk_sectors -1 )) + bio_sectors)) <<9 ; -if (max <0 ) max =0 ; -if (max <= biovec->bv_len && bio_sectors ==0 ) -return biovec->bv_len; -else -return max; -} -static int in_chunk_boundary (struct mddev *mddev,struct bio *bio) {sector_t sector = bio->bi_iter.bi_sector +get_start_sect (bio->bi_bdev);
@@ -7763,7 +7734,6 @@ static struct md_personality raid6_personality = .quiesce = raid5_quiesce, .takeover = raid6_takeover, .congested = raid5_congested, - .mergeable_bvec = raid5_mergeable_bvec, };static struct md_personality raid5_personality = {
@@ -7787,7 +7757,6 @@ static struct md_personality raid5_personality = .quiesce = raid5_quiesce, .takeover = raid5_takeover, .congested = raid5_congested, - .mergeable_bvec = raid5_mergeable_bvec, }; static struct md_personality raid4_personality =
@@ -7812,7 +7781,6 @@ static struct md_personality raid4_personality = .quiesce = raid5_quiesce, .takeover = raid4_takeover, .congested = raid5_congested, - .mergeable_bvec = raid5_mergeable_bvec, };static int __init raid5_init (void )
include/linux/blkdev.h +0 -10
@@ -213,14 +213,6 @@ typedef int (prep_rq_fn) (struct request_queue *, struct request *);typedef void (unprep_rq_fn ) (struct request_queue *,struct request *);struct bio_vec ; -struct bvec_merge_data { -struct block_device *bi_bdev; -sector_t bi_sector; -unsigned bi_size; -unsigned long bi_rw; -}; -typedef int (merge_bvec_fn ) (struct request_queue *,struct bvec_merge_data *, -struct bio_vec *);typedef void (softirq_done_fn )(struct request *);typedef int (dma_drain_needed_fn )(struct request *);typedef int (lld_busy_fn ) (struct request_queue *q);
@@ -305,7 +297,6 @@ struct request_queue {make_request_fn *make_request_fn;prep_rq_fn *prep_rq_fn;unprep_rq_fn *unprep_rq_fn; -merge_bvec_fn *merge_bvec_fn;softirq_done_fn *softirq_done_fn;rq_timed_out_fn *rq_timed_out_fn;dma_drain_needed_fn *dma_drain_needed;
@@ -991,7 +982,6 @@ extern void blk_queue_lld_busy(struct request_queue *q, lld_busy_fn *fn);extern void blk_queue_segment_boundary (struct request_queue *,unsigned long );extern void blk_queue_prep_rq (struct request_queue *,prep_rq_fn *pfn);extern void blk_queue_unprep_rq (struct request_queue *,unprep_rq_fn *ufn); -extern void blk_queue_merge_bvec (struct request_queue *,merge_bvec_fn *);extern void blk_queue_dma_alignment (struct request_queue *,int );extern void blk_queue_update_dma_alignment (struct request_queue *,int );extern void blk_queue_softirq_done (struct request_queue *,softirq_done_fn *);
include/linux/device-mapper.h +0 -4
@@ -82,9 +82,6 @@ typedef int (*dm_message_fn) (struct dm_target *ti, unsigned argc, char **argv);typedef int (*dm_ioctl_fn ) (struct dm_target *ti,unsigned int cmd,unsigned long arg); -typedef int (*dm_merge_fn ) (struct dm_target *ti,struct bvec_merge_data *bvm, -struct bio_vec *biovec,int max_size); -/* * These iteration functions are typically used to check (and combine) * properties of underlying devices.
@@ -160,7 +157,6 @@ struct target_type {dm_status_fn status;dm_message_fn message;dm_ioctl_fn ioctl; -dm_merge_fn merge;dm_busy_fn busy;dm_iterate_devices_fn iterate_devices;dm_io_hints_fn io_hints;
Signature · 1 line
1.9.1
PATCH v5 9/11 fs: use helper bio_add_page() instead of open coding on bi_io_vec
In reply to Ming LinFrom: Kent Overstreet <[email protected]>
Call pre-defined helper bio_add_page() instead of open coding for iterating through bi_io_vec[]. Doing that, it's possible to make some parts in filesystems and mm/page_io.c simpler than before.
--- fs/buffer.c | 7 ++----- fs/jfs/jfs_logmgr.c | 14 ++++---------- mm/page_io.c | 8 +++----- 3 files changed, 9 insertions(+), 20 deletions(-)
fs/buffer.c +2 -5
@@ -3046,12 +3046,9 @@ static int submit_bh_wbc(int rw, struct buffer_head *bh, bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >>9 ); bio->bi_bdev = bh->b_bdev; - bio->bi_io_vec[0 ].bv_page = bh->b_page; - bio->bi_io_vec[0 ].bv_len = bh->b_size; - bio->bi_io_vec[0 ].bv_offset =bh_offset (bh); - bio->bi_vcnt =1 ; - bio->bi_iter.bi_size = bh->b_size; +bio_add_page (bio, bh->b_page, bh->b_size,bh_offset (bh)); +BUG_ON (bio->bi_iter.bi_size != bh->b_size); bio->bi_end_io = end_bio_bh_io_sync; bio->bi_private = bh;
fs/jfs/jfs_logmgr.c +4 -10
@@ -1999,12 +1999,9 @@ static int lbmRead(struct jfs_log * log, int pn, struct lbuf ** bpp) bio->bi_iter.bi_sector = bp->l_blkno << (log->l2bsize -9 ); bio->bi_bdev = log->bdev; - bio->bi_io_vec[0 ].bv_page = bp->l_page; - bio->bi_io_vec[0 ].bv_len =LOGPSIZE ; - bio->bi_io_vec[0 ].bv_offset = bp->l_offset; - bio->bi_vcnt =1 ; - bio->bi_iter.bi_size =LOGPSIZE ; +bio_add_page (bio, bp->l_page,LOGPSIZE , bp->l_offset); +BUG_ON (bio->bi_iter.bi_size !=LOGPSIZE ); bio->bi_end_io = lbmIODone; bio->bi_private = bp;
@@ -2145,12 +2142,9 @@ static void lbmStartIO(struct lbuf * bp) bio =bio_alloc (GFP_NOFS ,1 ); bio->bi_iter.bi_sector = bp->l_blkno << (log->l2bsize -9 ); bio->bi_bdev = log->bdev; - bio->bi_io_vec[0 ].bv_page = bp->l_page; - bio->bi_io_vec[0 ].bv_len =LOGPSIZE ; - bio->bi_io_vec[0 ].bv_offset = bp->l_offset; - bio->bi_vcnt =1 ; - bio->bi_iter.bi_size =LOGPSIZE ; +bio_add_page (bio, bp->l_page,LOGPSIZE , bp->l_offset); +BUG_ON (bio->bi_iter.bi_size !=LOGPSIZE ); bio->bi_end_io = lbmIODone; bio->bi_private = bp;
mm/page_io.c +3 -5
@@ -33,12 +33,10 @@ static struct bio *get_swap_bio(gfp_t gfp_flags,if (bio) { bio->bi_iter.bi_sector =map_swap_page (page, &bio->bi_bdev); bio->bi_iter.bi_sector <<=PAGE_SHIFT -9 ; - bio->bi_io_vec[0 ].bv_page = page; - bio->bi_io_vec[0 ].bv_len =PAGE_SIZE ; - bio->bi_io_vec[0 ].bv_offset =0 ; - bio->bi_vcnt =1 ; - bio->bi_iter.bi_size =PAGE_SIZE ; bio->bi_end_io = end_io; + +bio_add_page (bio, page,PAGE_SIZE ,0 ); +BUG_ON (bio->bi_iter.bi_size !=PAGE_SIZE ); }return bio; }
Signature · 1 line
1.9.1
PATCH v5 10/11 block: remove bio_get_nr_vecs()
In reply to Ming LinFrom: Kent Overstreet <[email protected]>
We can always fill up the bio now, no need to estimate the possible size based on queue parameters.
--- block/bio.c | 23 ----------------------- drivers/md/dm-io.c | 2 +- fs/btrfs/compression.c | 5 +---- fs/btrfs/extent_io.c | 9 ++------- fs/btrfs/inode.c | 3 +-- fs/btrfs/scrub.c | 18 ++---------------- fs/direct-io.c | 2 +- fs/ext4/page-io.c | 3 +-- fs/ext4/readpage.c | 2 +- fs/f2fs/data.c | 2 +- fs/gfs2/lops.c | 9 +-------- fs/logfs/dev_bdev.c | 4 ++-- fs/mpage.c | 4 ++-- fs/nilfs2/segbuf.c | 2 +- fs/xfs/xfs_aops.c | 3 +-- include/linux/bio.h | 1 - 16 files changed, 18 insertions(+), 74 deletions(-)
block/bio.c +0 -23
@@ -692,29 +692,6 @@ integrity_clone:EXPORT_SYMBOL (bio_clone_bioset);/** -* bio_get_nr_vecs - return approx number of vecs -* @bdev: I/O target -* -* Return the approximate number of pages we can send to this target. -* There's no guarantee that you will be able to fit this number of pages -* into a bio, it does not account for dynamic restrictions that vary -* on offset. -*/ -int bio_get_nr_vecs (struct block_device *bdev) -{ -struct request_queue *q =bdev_get_queue (bdev); -int nr_pages; - - nr_pages =min_t (unsigned, -queue_max_segments (q), -queue_max_sectors (q) / (PAGE_SIZE >>9 ) +1 ); - -return min_t (unsigned, nr_pages,BIO_MAX_PAGES ); - -} -EXPORT_SYMBOL (bio_get_nr_vecs); - -/** * bio_add_pc_page - attempt to add page to bio * @q: the target queue * @bio: destination bio
drivers/md/dm-io.c +1 -1
@@ -314,7 +314,7 @@ static void do_region(int rw, unsigned region, struct dm_io_region *where,if ((rw &REQ_DISCARD ) || (rw &REQ_WRITE_SAME )) num_bvecs =1 ;else - num_bvecs =min_t (int,bio_get_nr_vecs (where->bdev), + num_bvecs =min_t (int,BIO_MAX_PAGES ,dm_sector_div_up (remaining, (PAGE_SIZE >>SECTOR_SHIFT ))); bio =bio_alloc_bioset (GFP_NOIO , num_bvecs, io->client->bios);
fs/btrfs/compression.c +1 -4
@@ -97,10 +97,7 @@ static inline int compressed_bio_size(struct btrfs_root *root,static struct bio *compressed_bio_alloc (struct block_device *bdev,u64 first_byte,gfp_t gfp_flags) { -int nr_vecs; - - nr_vecs =bio_get_nr_vecs (bdev); -return btrfs_bio_alloc (bdev, first_byte >>9 , nr_vecs, gfp_flags); +return btrfs_bio_alloc (bdev, first_byte >>9 ,BIO_MAX_PAGES , gfp_flags); } static int check_compressed_csum(struct inode *inode,
fs/btrfs/extent_io.c +2 -7
@@ -2802,9 +2802,7 @@ static int submit_extent_page(int rw, struct extent_io_tree *tree, {int ret =0 ;struct bio *bio; -int nr;int contig =0 ; -int this_compressed = bio_flags &EXTENT_BIO_COMPRESSED ;int old_compressed = prev_bio_flags &EXTENT_BIO_COMPRESSED ;size_t page_size =min_t (size_t, size,PAGE_CACHE_SIZE );
@@ -2829,12 +2827,9 @@ static int submit_extent_page(int rw, struct extent_io_tree *tree,return 0 ; } } -if (this_compressed) - nr =BIO_MAX_PAGES ; -else - nr =bio_get_nr_vecs (bdev); - bio =btrfs_bio_alloc (bdev, sector, nr,GFP_NOFS | __GFP_HIGH); + bio =btrfs_bio_alloc (bdev, sector,BIO_MAX_PAGES , +GFP_NOFS | __GFP_HIGH);if (!bio)return -ENOMEM ;
fs/btrfs/inode.c +1 -2
@@ -7959,8 +7959,7 @@ out:static struct bio *btrfs_dio_bio_alloc (struct block_device *bdev,u64 first_sector,gfp_t gfp_flags) { -int nr_vecs =bio_get_nr_vecs (bdev); -return btrfs_bio_alloc (bdev, first_sector, nr_vecs, gfp_flags); +return btrfs_bio_alloc (bdev, first_sector,BIO_MAX_PAGES , gfp_flags); } static inline int btrfs_lookup_and_bind_dio_csum(struct btrfs_root *root,
fs/btrfs/scrub.c +2 -16
@@ -454,27 +454,14 @@ struct scrub_ctx *scrub_setup_ctx(struct btrfs_device *dev, int is_dev_replace)struct scrub_ctx *sctx;int i;struct btrfs_fs_info *fs_info = dev->dev_root->fs_info; -int pages_per_rd_bio;int ret; -/* -* the setting of pages_per_rd_bio is correct for scrub but might -* be wrong for the dev_replace code where we might read from -* different devices in the initial huge bios. However, that -* code is able to correctly handle the case when adding a page -* to a bio fails. -*/ -if (dev->bdev) - pages_per_rd_bio =min_t (int,SCRUB_PAGES_PER_RD_BIO , -bio_get_nr_vecs (dev->bdev)); -else - pages_per_rd_bio =SCRUB_PAGES_PER_RD_BIO ; sctx =kzalloc (sizeof (*sctx),GFP_NOFS );if (!sctx) goto nomem;atomic_set (&sctx->refs,1 ); sctx->is_dev_replace = is_dev_replace; - sctx->pages_per_rd_bio = pages_per_rd_bio; + sctx->pages_per_rd_bio =SCRUB_PAGES_PER_RD_BIO ; sctx->curr =-1 ; sctx->dev_root = dev->dev_root;for (i =0 ; i <SCRUB_BIOS_PER_SCTX ; ++i) {
@@ -3895,8 +3882,7 @@ static int scrub_setup_wr_ctx(struct scrub_ctx *sctx,return 0 ;WARN_ON (!dev->bdev); - wr_ctx->pages_per_wr_bio =min_t (int,SCRUB_PAGES_PER_WR_BIO , -bio_get_nr_vecs (dev->bdev)); + wr_ctx->pages_per_wr_bio =SCRUB_PAGES_PER_WR_BIO ; wr_ctx->tgtdev = dev;atomic_set (&wr_ctx->flush_all_writes,0 );return 0 ;
fs/direct-io.c +1 -1
@@ -653,7 +653,7 @@ static inline int dio_new_bio(struct dio *dio, struct dio_submit *sdio,if (ret) goto out; sector = start_sector << (sdio->blkbits -9 ); - nr_pages =min (sdio->pages_in_io,bio_get_nr_vecs (map_bh->b_bdev)); + nr_pages =min (sdio->pages_in_io,BIO_MAX_PAGES );BUG_ON (nr_pages <=0 );dio_bio_alloc (dio, sdio, map_bh->b_bdev, sector, nr_pages); sdio->boundary =0 ;
fs/ext4/page-io.c +1 -2
@@ -375,10 +375,9 @@ void ext4_io_submit_init(struct ext4_io_submit *io,static int io_submit_init_bio (struct ext4_io_submit *io,struct buffer_head *bh) { -int nvecs =bio_get_nr_vecs (bh->b_bdev);struct bio *bio; - bio =bio_alloc (GFP_NOIO ,min (nvecs,BIO_MAX_PAGES )); + bio =bio_alloc (GFP_NOIO ,BIO_MAX_PAGES );if (!bio)return -ENOMEM ; bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >>9 );
fs/ext4/readpage.c +1 -1
@@ -284,7 +284,7 @@ int ext4_mpage_readpages(struct address_space *mapping, goto set_error_page; } bio =bio_alloc (GFP_KERNEL , -min_t (int, nr_pages,bio_get_nr_vecs (bdev))); +min_t (int, nr_pages,BIO_MAX_PAGES ));if (!bio) {if (ctx)ext4_release_crypto_ctx (ctx);
fs/f2fs/data.c +1 -1
@@ -1552,7 +1552,7 @@ submit_and_realloc: } bio =bio_alloc (GFP_KERNEL , -min_t (int, nr_pages,bio_get_nr_vecs (bdev))); +min_t (int, nr_pages,BIO_MAX_PAGES ));if (!bio) {if (ctx)f2fs_release_crypto_ctx (ctx);
fs/gfs2/lops.c +1 -8
@@ -261,18 +261,11 @@ void gfs2_log_flush_bio(struct gfs2_sbd *sdp, int rw)static struct bio *gfs2_log_alloc_bio (struct gfs2_sbd *sdp,u64 blkno) {struct super_block *sb = sdp->sd_vfs; -unsigned nrvecs =bio_get_nr_vecs (sb->s_bdev);struct bio *bio;BUG_ON (sdp->sd_log_bio); -while (1 ) { - bio =bio_alloc (GFP_NOIO , nrvecs); -if (likely (bio)) -break ; - nrvecs =max (nrvecs/2 ,1U ); - } - + bio =bio_alloc (GFP_NOIO ,BIO_MAX_PAGES ); bio->bi_iter.bi_sector = blkno * (sb->s_blocksize >>9 ); bio->bi_bdev = sb->s_bdev; bio->bi_end_io = gfs2_end_log_write;
fs/logfs/dev_bdev.c +2 -2
@@ -83,7 +83,7 @@ static int __bdev_writeseg(struct super_block *sb, u64 ofs, pgoff_t index,unsigned int max_pages;int i; - max_pages =min (nr_pages, (size_t )bio_get_nr_vecs (super->s_bdev)); + max_pages =min (nr_pages,BIO_MAX_PAGES ); bio =bio_alloc (GFP_NOFS , max_pages);BUG_ON (!bio);
@@ -175,7 +175,7 @@ static int do_erase(struct super_block *sb, u64 ofs, pgoff_t index,unsigned int max_pages;int i; - max_pages =min (nr_pages, (size_t )bio_get_nr_vecs (super->s_bdev)); + max_pages =min (nr_pages,BIO_MAX_PAGES ); bio =bio_alloc (GFP_NOFS , max_pages);BUG_ON (!bio);
fs/mpage.c +2 -2
@@ -277,7 +277,7 @@ alloc_new: goto out; } bio =mpage_alloc (bdev, blocks[0 ] << (blkbits -9 ), -min_t (int, nr_pages,bio_get_nr_vecs (bdev)), +min_t (int, nr_pages,BIO_MAX_PAGES ),GFP_KERNEL );if (bio ==NULL ) goto confused;
@@ -602,7 +602,7 @@ alloc_new: } } bio =mpage_alloc (bdev, blocks[0 ] << (blkbits -9 ), -bio_get_nr_vecs (bdev),GFP_NOFS |__GFP_HIGH); +BIO_MAX_PAGES ,GFP_NOFS |__GFP_HIGH);if (bio ==NULL ) goto confused;
fs/nilfs2/segbuf.c +1 -1
@@ -415,7 +415,7 @@ static void nilfs_segbuf_prepare_write(struct nilfs_segment_buffer *segbuf, { wi->bio =NULL ; wi->rest_blocks = segbuf->sb_sum.nblocks; - wi->max_pages =bio_get_nr_vecs (wi->nilfs->ns_bdev); + wi->max_pages =BIO_MAX_PAGES ; wi->nr_vecs =min (wi->max_pages, wi->rest_blocks); wi->start = wi->end =0 ; wi->blocknr = segbuf->sb_pseg_start;
fs/xfs/xfs_aops.c +1 -2
@@ -382,8 +382,7 @@ STATIC struct bio *xfs_alloc_ioend_bio ( struct buffer_head *bh) { -int nvecs =bio_get_nr_vecs (bh->b_bdev); -struct bio *bio =bio_alloc (GFP_NOIO , nvecs); +struct bio *bio =bio_alloc (GFP_NOIO ,BIO_MAX_PAGES );ASSERT (bio->bi_private ==NULL ); bio->bi_iter.bi_sector = bh->b_blocknr * (bh->b_size >>9 );
include/linux/bio.h +0 -1
@@ -440,7 +440,6 @@ void bio_chain(struct bio *, struct bio *);extern int bio_add_page (struct bio *,struct page *,unsigned int ,unsigned int );extern int bio_add_pc_page (struct request_queue *,struct bio *,struct page *,unsigned int ,unsigned int ); -extern int bio_get_nr_vecs (struct block_device *);struct rq_map_data ;extern struct bio *bio_map_user_iov (struct request_queue *,const struct iov_iter *,gfp_t );
Signature · 1 line
1.9.1
Re: [PATCH v5 10/11] block: remove bio_get_nr_vecs()
Hi,
Quoted 4 lines · Ming Lin
From: Kent Overstreet <[email protected]>
We can always fill up the bio now, no need to estimate the possible size based on queue parameters.
[snip]
Quoted 14 lines
fs/gfs2/lops.c +0
-2@@ -261,18 +261,11 @@ void gfs2_log_flush_bio(struct gfs2_sbd *sdp, int rw)static struct bio *gfs2_log_alloc_bio (struct gfs2_sbd *sdp,u64 blkno) {struct super_block *sb = sdp->sd_vfs; -unsigned nrvecs =bio_get_nr_vecs (sb->s_bdev);struct bio *bio;BUG_ON (sdp->sd_log_bio); -while (1 ) {
fs/gfs2/lops.c void gfs2_log_flush_bio(struct gfs2_sbd *sdp, int rw) · line 271
Quoted 10 lines
fs/gfs2/lops.c +1
-6- bio =bio_alloc (GFP_NOIO , nrvecs); -if (likely (bio)) -break ; - nrvecs =max (nrvecs/2 ,1U ); - } - + bio =bio_alloc (GFP_NOIO ,BIO_MAX_PAGES ); bio->bi_iter.bi_sector = blkno * (sb->s_blocksize >>9 ); bio->bi_bdev = sb->s_bdev; bio->bi_end_io = gfs2_end_log_write;
When I wrote this loop I understood that bio_alloc(GFP_NOIO, x) was only guaranteed to be successful in the case x = 1, and that for x > 1, it might return NULL. Does this now mean that for x = BIO_MAX_PAGES it will be guaranteed to never return NULL in this case?
Steve.
Re: [PATCH v5 10/11] block: remove bio_get_nr_vecs()
Quoted 30 lines · Steven Whitehouse
Hi,
Quoted 5 lines · Ming Lin
From: Kent Overstreet <[email protected]>
We can always fill up the bio now, no need to estimate the possible size based on queue parameters.
[snip]
Quoted 18 lines
fs/gfs2/lops.c +0
-6@@ -261,18 +261,11 @@ void gfs2_log_flush_bio(struct gfs2_sbd *sdp, int rw)static struct bio *gfs2_log_alloc_bio (struct gfs2_sbd *sdp,u64 blkno) {struct super_block *sb = sdp->sd_vfs; -unsigned nrvecs =bio_get_nr_vecs (sb->s_bdev);struct bio *bio;BUG_ON (sdp->sd_log_bio); - while (1) { - bio =bio_alloc (GFP_NOIO , nrvecs); -if (likely (bio)) -break ; - nrvecs =max (nrvecs/2 ,1U ); - }
Quoted 8 lines
fs/gfs2/lops.c void gfs2_log_flush_bio(struct gfs2_sbd *sdp, int rw) · line 271
Quoted 5 lines
fs/gfs2/lops.c +1
-1- + bio =bio_alloc (GFP_NOIO ,BIO_MAX_PAGES ); bio->bi_iter.bi_sector = blkno * (sb->s_blocksize >>9 ); bio->bi_bdev = sb->s_bdev; bio->bi_end_io = gfs2_end_log_write;When I wrote this loop I understood that
bio_alloc(GFP_NOIO, x)was only guaranteed to be successful in the case x = 1, and that for x > 1, it might return NULL. Does this now mean that for x =BIO_MAX_PAGESit will be guaranteed to never return NULL in this case?
Yes, it's guaranteed.
https://lkml.org/lkml/2013/11/26/47
commit df2cb6daa4cbc
Steven Whitehouse
Acked-byRe: [PATCH v5 10/11] block: remove bio_get_nr_vecs()
Hi,
Quoted 34 lines · Ming Lin
Quoted 25 lines · Steven Whitehouse
Hi,
Quoted 4 lines · Ming Lin
From: Kent Overstreet <[email protected]>
We can always fill up the bio now, no need to estimate the possible size based on queue parameters.
[snip]
Quoted 16 lines
fs/gfs2/lops.c +0
-5@@ -261,18 +261,11 @@ void gfs2_log_flush_bio(struct gfs2_sbd *sdp, int rw)static struct bio *gfs2_log_alloc_bio (struct gfs2_sbd *sdp,u64 blkno) {struct super_block *sb = sdp->sd_vfs; -unsigned nrvecs =bio_get_nr_vecs (sb->s_bdev);struct bio *bio;BUG_ON (sdp->sd_log_bio); - while (1) { - bio =bio_alloc (GFP_NOIO , nrvecs); -if (likely (bio)) -break ; - nrvecs =max (nrvecs/2 ,1U );Quoted 8 lines
fs/gfs2/lops.c void gfs2_log_flush_bio(struct gfs2_sbd *sdp, int rw) · line 271
Quoted 6 lines
fs/gfs2/lops.c +1
-2- } - + bio =bio_alloc (GFP_NOIO ,BIO_MAX_PAGES ); bio->bi_iter.bi_sector = blkno * (sb->s_blocksize >>9 ); bio->bi_bdev = sb->s_bdev; bio->bi_end_io = gfs2_end_log_write;When I wrote this loop I understood that
bio_alloc(GFP_NOIO, x)was only guaranteed to be successful in the case x = 1, and that for x > 1, it might return NULL. Does this now mean that for x =BIO_MAX_PAGESit will be guaranteed to never return NULL in this case?
Quoted 5 lines
Yes, it's guaranteed.
https://lkml.org/lkml/2013/11/26/47
commit
df2cb6daa4cbc
Ok. Feel free to add:
Steve.
PATCH v5 11/11 Documentation: update notes in biovecs about arbitrarily sized bios
In reply to Ming LinFrom: Dongsu Park <[email protected]>
Update block/biovecs.txt so that it includes a note on what kind of effects arbitrarily sized bios would bring to the block layer. Also fix a trivial typo, bio_iter_iovec.
--- Documentation/block/biovecs.txt | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)
Documentation/block/biovecs.txt +9 -1
@@ -24,7 +24,7 @@ particular, presenting the illusion of partially completed biovecs so that normal code doesn't have to deal with bi_bvec_done. * Driver code should no longer refer to biovecs directly; we now have - bio_iovec() and bio_iovec_iter() macros that return literal struct biovecs, + bio_iovec() and bio_iter_iovec() macros that return literal struct biovecs, constructed from the raw biovecs but taking into account bi_bvec_done and bi_size.
@@ -109,3 +109,11 @@ Other implications: over all the biovecs in the new bio - which is silly as it's not needed. So, don't use bi_vcnt anymore. + + * The current interface allows the block layer to split bios as needed, so we + could eliminate a lot of complexity particularly in stacked drivers. Code + that creates bios can then create whatever size bios are convenient, and + more importantly stacked drivers don't have to deal with both their own bio + size limitations and the limitations of the underlying devices. Thus + there's no need to define ->merge_bvec_fn() callbacks for individual block + drivers.
Signature · 1 line
1.9.1
Quoted 63 lines · Ming Lin
From: Kent Overstreet <[email protected]>
The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as
bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete
merge_bvec_fnand a bunch of other code.We do this by adding calls to
blk_queue_split()to the variousmake_requestfunctions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call toblk_queue_bounce(); this means thatblk_queue_split()andblk_recalc_rq_segments()don't need to be concerned with bouncing affecting segment merging.Some
make_request_fn()callbacks were simple enough to audit and verify they don't needblk_queue_split()calls. The skipped ones are:*
nfhd_make_request(arch/m68k/emu/nfblock.c)
*axon_ram_make_request(arch/powerpc/sysdev/axonram.c)
*simdisk_make_request(arch/xtensa/platforms/iss/simdisk.c)
*brd_make_request(ramdisk -drivers/block/brd.c)
*mtip_submit_request(drivers/block/mtip32xx/mtip32xx.c)
*loop_make_request
*null_queue_bio
* bcache'smake_requestfnsSome others are almost certainly safe to remove now, but will be left for future patches.
Cc Jens Axboe <[email protected]>Cc Christoph Hellwig <[email protected]>Cc Al Viro <[email protected]>Cc Ming Lei <[email protected]>Cc Neil Brown <[email protected]>Cc Alasdair Kergon <[email protected]>Cc Mike Snitzer <[email protected]>Cc Lars Ellenberg <[email protected]>Cc Jiri Kosina <[email protected]>Cc Geoff Levand <[email protected]>Cc Jim Paris <[email protected]>Cc Joshua Morris <[email protected]>Cc Philip Kelleher <[email protected]>Cc Minchan Kim <[email protected]>Cc Nitin Gupta <[email protected]>Cc Oleg Drokin <[email protected]>Cc Andreas Dilger <[email protected]>Signed-off-by Kent Overstreet <[email protected]>[dpark: skip more mq-based drivers, resolve merge conflicts, etc.]Signed-off-by Dongsu Park <[email protected]>Signed-off-by Ming Lin <[email protected]>
...
Quoted 37 lines
block/blk-merge.c +29
-0@@ -9,12 +9,158 @@#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. +*/
block/blk-merge.c line 50
Quoted 10 lines
block/blk-merge.c +10
-0+ 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); +}
This code to stop the discard at the previous aligned sector could be the reason why I have 2 device-mapper-test-suite tests in the 'thin-provisioning' testsuite failing due to this patchset:
4) Failure:test_discard_a_fragmented_device(DiscardQuickTests)
[/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:49:in `block in assert_no_mappings'
/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:in call' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:in block in with_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:in each' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:in with_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:48:in assert_no_mappings' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:263:in test_discard_a_fragmented_device']:
<0> expected but was
<131072>.
5) Failure:test_discard_fully_provisioned_device_benchmark(DiscardQuickTests)
[/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:49:in `block in assert_no_mappings'
/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:in call' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:in block in with_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:in each' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:in with_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:48:in assert_no_mappings' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:225:in test_discard_fully_provisioned_device_benchmark']:
<0> expected but was
<262144>.
But I haven't yet figured out where the blk_bio_discard_split code, or elsewhere, is going wrong.
Mike
Quoted 110 lines · Mike Snitzer
Quoted 63 lines · Ming Lin
From: Kent Overstreet <[email protected]>
The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as
bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete
merge_bvec_fnand a bunch of other code.We do this by adding calls to
blk_queue_split()to the variousmake_requestfunctions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call toblk_queue_bounce(); this means thatblk_queue_split()andblk_recalc_rq_segments()don't need to be concerned with bouncing affecting segment merging.Some
make_request_fn()callbacks were simple enough to audit and verify they don't needblk_queue_split()calls. The skipped ones are:*
nfhd_make_request(arch/m68k/emu/nfblock.c)
*axon_ram_make_request(arch/powerpc/sysdev/axonram.c)
*simdisk_make_request(arch/xtensa/platforms/iss/simdisk.c)
*brd_make_request(ramdisk -drivers/block/brd.c)
*mtip_submit_request(drivers/block/mtip32xx/mtip32xx.c)
*loop_make_request
*null_queue_bio
* bcache'smake_requestfnsSome others are almost certainly safe to remove now, but will be left for future patches.
Cc Jens Axboe <axboe-tSWWG44O7X1aa/[email protected]>Cc Christoph Hellwig <[email protected]>Cc Al Viro <viro-RmSDqhL/[email protected]>Cc Ming Lei <[email protected]>Cc Neil Brown <[email protected]>Cc Alasdair Kergon <[email protected]>Cc Mike Snitzer <[email protected]>Cc Lars Ellenberg <[email protected]>Cc Jiri Kosina <[email protected]>Cc Geoff Levand <[email protected]>Cc Jim Paris <jim-XrPbb/[email protected]>Cc Joshua Morris <josh.h.morris-r/[email protected]>Cc Philip Kelleher <[email protected]>Cc Minchan Kim <[email protected]>Cc Nitin Gupta <[email protected]>Cc Oleg Drokin <[email protected]>Cc Andreas Dilger <[email protected]>Signed-off-by Kent Overstreet <[email protected]>[dpark: skip more mq-based drivers, resolve merge conflicts, etc.]Signed-off-by Dongsu Park <dpark-VwIFZPTo/[email protected]>Signed-off-by Ming Lin <[email protected]>...
Quoted 43 lines
block/blk-merge.c +35
-0@@ -9,12 +9,158 @@#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)
Quoted 8 lines
block/blk-merge.c line 50
Quoted 4 lines
block/blk-merge.c +4
-0+ split_sectors -= tmp; + +return bio_split (bio, split_sectors,GFP_NOIO , bs); +}This code to stop the discard at the previous aligned sector could be the reason why I have 2 device-mapper-test-suite tests in the 'thin-provisioning' testsuite failing due to this patchset:
I'm setting up the testsuite to debug.
Ming
Quoted 29 lines
4) Failure:
test_discard_a_fragmented_device(DiscardQuickTests)
[/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:49:in `block inassert_no_mappings'
/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:incall' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:inblock inwith_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:ineach' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:inwith_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:48:inassert_no_mappings' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:263:intest_discard_a_fragmented_device']:
<0> expected but was
<131072>.5) Failure:
test_discard_fully_provisioned_device_benchmark(DiscardQuickTests)
[/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:49:in `block inassert_no_mappings'
/root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:incall' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:43:inblock inwith_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:ineach' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:40:inwith_dev_md' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:48:inassert_no_mappings' /root/snitm/git/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:225:intest_discard_fully_provisioned_device_benchmark']:
<0> expected but was
<262144>.But I haven't yet figured out where the
blk_bio_discard_splitcode, or elsewhere, is going wrong.Mike
Quoted 120 lines · Ming Lin
Quoted 110 lines · Mike Snitzer
Quoted 63 lines · Ming Lin
From: Kent Overstreet <[email protected]>
The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as
bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete
merge_bvec_fnand a bunch of other code.We do this by adding calls to
blk_queue_split()to the variousmake_requestfunctions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call toblk_queue_bounce(); this means thatblk_queue_split()andblk_recalc_rq_segments()don't need to be concerned with bouncing affecting segment merging.Some
make_request_fn()callbacks were simple enough to audit and verify they don't needblk_queue_split()calls. The skipped ones are:*
nfhd_make_request(arch/m68k/emu/nfblock.c)
*axon_ram_make_request(arch/powerpc/sysdev/axonram.c)
*simdisk_make_request(arch/xtensa/platforms/iss/simdisk.c)
*brd_make_request(ramdisk -drivers/block/brd.c)
*mtip_submit_request(drivers/block/mtip32xx/mtip32xx.c)
*loop_make_request
*null_queue_bio
* bcache'smake_requestfnsSome others are almost certainly safe to remove now, but will be left for future patches.
Cc Jens Axboe <axboe-tSWWG44O7X1aa/[email protected]>Cc Christoph Hellwig <[email protected]>Cc Al Viro <viro-RmSDqhL/[email protected]>Cc Ming Lei <[email protected]>Cc Neil Brown <[email protected]>Cc Alasdair Kergon <[email protected]>Cc Mike Snitzer <[email protected]>Cc Lars Ellenberg <[email protected]>Cc Jiri Kosina <[email protected]>Cc Geoff Levand <[email protected]>Cc Jim Paris <jim-XrPbb/[email protected]>Cc Joshua Morris <josh.h.morris-r/[email protected]>Cc Philip Kelleher <[email protected]>Cc Minchan Kim <[email protected]>Cc Nitin Gupta <[email protected]>Cc Oleg Drokin <[email protected]>Cc Andreas Dilger <[email protected]>Signed-off-by Kent Overstreet <[email protected]>[dpark: skip more mq-based drivers, resolve merge conflicts, etc.]Signed-off-by Dongsu Park <dpark-VwIFZPTo/[email protected]>Signed-off-by Ming Lin <[email protected]>...
Quoted 43 lines
block/blk-merge.c +35
-0@@ -9,12 +9,158 @@#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)Quoted 8 lines
block/blk-merge.c line 50
Quoted 4 lines
block/blk-merge.c +4
-0+ split_sectors -= tmp; + +return bio_split (bio, split_sectors,GFP_NOIO , bs); +}This code to stop the discard at the previous aligned sector could be the reason why I have 2 device-mapper-test-suite tests in the 'thin-provisioning' testsuite failing due to this patchset:
Quoted 1 line
I'm setting up the testsuite to debug.
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.
Quoted 125 lines · Mike Snitzer
Quoted 120 lines · Ming Lin
Quoted 110 lines · Mike Snitzer
Quoted 63 lines · Ming Lin
From: Kent Overstreet <[email protected]>
The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as
bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete
merge_bvec_fnand a bunch of other code.We do this by adding calls to
blk_queue_split()to the variousmake_requestfunctions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call toblk_queue_bounce(); this means thatblk_queue_split()andblk_recalc_rq_segments()don't need to be concerned with bouncing affecting segment merging.Some
make_request_fn()callbacks were simple enough to audit and verify they don't needblk_queue_split()calls. The skipped ones are:*
nfhd_make_request(arch/m68k/emu/nfblock.c)
*axon_ram_make_request(arch/powerpc/sysdev/axonram.c)
*simdisk_make_request(arch/xtensa/platforms/iss/simdisk.c)
*brd_make_request(ramdisk -drivers/block/brd.c)
*mtip_submit_request(drivers/block/mtip32xx/mtip32xx.c)
*loop_make_request
*null_queue_bio
* bcache'smake_requestfnsSome others are almost certainly safe to remove now, but will be left for future patches.
Cc Jens Axboe <[email protected]>Cc Christoph Hellwig <[email protected]>Cc Al Viro <[email protected]>Cc Ming Lei <[email protected]>Cc Neil Brown <[email protected]>Cc Alasdair Kergon <[email protected]>Cc Mike Snitzer <[email protected]>Cc Lars Ellenberg <[email protected]>Cc Jiri Kosina <[email protected]>Cc Geoff Levand <[email protected]>Cc Jim Paris <[email protected]>Cc Joshua Morris <[email protected]>Cc Philip Kelleher <[email protected]>Cc Minchan Kim <[email protected]>Cc Nitin Gupta <[email protected]>Cc Oleg Drokin <[email protected]>Cc Andreas Dilger <[email protected]>Signed-off-by Kent Overstreet <[email protected]>[dpark: skip more mq-based drivers, resolve merge conflicts, etc.]Signed-off-by Dongsu Park <[email protected]>Signed-off-by Ming Lin <[email protected]>...
Quoted 43 lines
block/blk-merge.c +35
-0@@ -9,12 +9,158 @@#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)Quoted 8 lines
block/blk-merge.c line 50
Quoted 4 lines
block/blk-merge.c +4
-0+ split_sectors -= tmp; + +return bio_split (bio, split_sectors,GFP_NOIO , bs); +}This code to stop the discard at the previous aligned sector could be the reason why I have 2 device-mapper-test-suite tests in the 'thin-provisioning' testsuite failing due to this patchset:
Quoted 1 line
I'm setting up the testsuite to debug.
Quoted 8 lines
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_devicedmtest run --suite thin-provisioning -n
discard_fully_provisioned_device_benchmarkAgain, these tests pass without this patchset.
(trim CC list)
I followed
https://github.com/jthornber/device-mapper-test-suite
https://github.com/jthornber/thin-provisioning-tools
to setup.
$ grep CONFIG_DM_ .config CONFIG_DM_MQ_DEFAULT=y CONFIG_DM_DEBUG=y CONFIG_DM_BUFIO=y CONFIG_DM_BIO_PRISON=y CONFIG_DM_PERSISTENT_DATA=y # CONFIG_DM_DEBUG_BLOCK_STACK_TRACING is not set CONFIG_DM_CRYPT=y CONFIG_DM_SNAPSHOT=y CONFIG_DM_THIN_PROVISIONING=y CONFIG_DM_CACHE=y CONFIG_DM_CACHE_MQ=y CONFIG_DM_CACHE_SMQ=y CONFIG_DM_CACHE_CLEANER=y CONFIG_DM_ERA=y CONFIG_DM_MIRROR=y CONFIG_DM_LOG_USERSPACE=y CONFIG_DM_RAID=y CONFIG_DM_ZERO=y CONFIG_DM_MULTIPATH=y CONFIG_DM_MULTIPATH_QL=y CONFIG_DM_MULTIPATH_ST=y CONFIG_DM_DELAY=y CONFIG_DM_UEVENT=y CONFIG_DM_FLAKEY=y CONFIG_DM_VERITY=y CONFIG_DM_SWITCH=y CONFIG_DM_LOG_WRITES=y
# cat .dmtest/config profile :ssd do metadata_dev '/dev/vda' data_dev '/dev/vdb' end # # profile :spindle do # metadata_dev '/dev/vdd' # data_dev '/dev/vde' # end # # profile :mix do # metadata_dev '/dev/vdb' # data_dev '/dev/vde' # end # # default_profile :ssd # default_test_scale :normal
root@bee:~/device-mapper-test-suite# dmtest run --suite thin-provisioning -n discard_a_fragmented_device Loaded suite thin-provisioning Started DiscardQuickTests discard_a_fragmented_device...restored metadata FAIL
Finished in 1.958143844 seconds.
1) Error:test_discard_a_fragmented_device(DiscardQuickTests):ProcessControl::ExitError: command failed: dmsetup load test-dev-307057 /tmp/dm-table20150731-3386-1rncpsb
/root/device-mapper-test-suite/lib/dmtest/process.rb:96:in wait' /root/device-mapper-test-suite/lib/dmtest/process.rb:111:in really_run' /root/device-mapper-test-suite/lib/dmtest/process.rb:116:in run_' /root/device-mapper-test-suite/lib/dmtest/process.rb:138:in run' /root/device-mapper-test-suite/lib/dmtest/device-mapper/interface.rb:16:in block in load' /root/device-mapper-test-suite/lib/dmtest/utils.rb:94:in with_temp_file' /root/device-mapper-test-suite/lib/dmtest/device-mapper/interface.rb:12:in load' /root/device-mapper-test-suite/lib/dmtest/device-mapper/dev.rb:19:in load' /root/device-mapper-test-suite/lib/dmtest/device-mapper/lexical_operators.rb:53:in block in create' /root/device-mapper-test-suite/lib/dmtest/prelude.rb:39:in protect_' /root/device-mapper-test-suite/lib/dmtest/device-mapper/lexical_operators.rb:47:in create' /root/device-mapper-test-suite/lib/dmtest/device-mapper/lexical_operators.rb:10:in with_dev' /root/device-mapper-test-suite/lib/dmtest/thinp-mixin.rb:147:in with_fake_discard' /root/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:109:in with_discardable_pool' /root/device-mapper-test-suite/lib/dmtest/tests/thin-provisioning/discard_tests.rb:253:in `test_discard_a_fragmented_device'
1 tests, 0 assertions, 0 failures, 1 errors
Tests failed
root@bee:~/device-mapper-test-suite# dmesg [ 83.824523] device-mapper: table: 249:0: fake-discard: unknown target type [ 83.827539] device-mapper: ioctl: error adding target to table
Seems my setup has problem. Could you help to take a look?
Quoted 173 lines · Ming Lin
Quoted 125 lines · Mike Snitzer
Quoted 120 lines · Ming Lin
Quoted 110 lines · Mike Snitzer
Quoted 63 lines · Ming Lin
From: Kent Overstreet <[email protected]>
The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as
bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete
merge_bvec_fnand a bunch of other code.We do this by adding calls to
blk_queue_split()to the variousmake_requestfunctions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call toblk_queue_bounce(); this means thatblk_queue_split()andblk_recalc_rq_segments()don't need to be concerned with bouncing affecting segment merging.Some
make_request_fn()callbacks were simple enough to audit and verify they don't needblk_queue_split()calls. The skipped ones are:*
nfhd_make_request(arch/m68k/emu/nfblock.c)
*axon_ram_make_request(arch/powerpc/sysdev/axonram.c)
*simdisk_make_request(arch/xtensa/platforms/iss/simdisk.c)
*brd_make_request(ramdisk -drivers/block/brd.c)
*mtip_submit_request(drivers/block/mtip32xx/mtip32xx.c)
*loop_make_request
*null_queue_bio
* bcache'smake_requestfnsSome others are almost certainly safe to remove now, but will be left for future patches.
Cc Jens Axboe <[email protected]>Cc Christoph Hellwig <[email protected]>Cc Al Viro <[email protected]>Cc Ming Lei <[email protected]>Cc Neil Brown <[email protected]>Cc Alasdair Kergon <[email protected]>Cc Mike Snitzer <[email protected]>Cc Lars Ellenberg <[email protected]>Cc Jiri Kosina <[email protected]>Cc Geoff Levand <[email protected]>Cc Jim Paris <[email protected]>Cc Joshua Morris <[email protected]>Cc Philip Kelleher <[email protected]>Cc Minchan Kim <[email protected]>Cc Nitin Gupta <[email protected]>Cc Oleg Drokin <[email protected]>Cc Andreas Dilger <[email protected]>Signed-off-by Kent Overstreet <[email protected]>[dpark: skip more mq-based drivers, resolve merge conflicts, etc.]Signed-off-by Dongsu Park <[email protected]>Signed-off-by Ming Lin <[email protected]>...
Quoted 43 lines
block/blk-merge.c +35
-0@@ -9,12 +9,158 @@#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)Quoted 8 lines
block/blk-merge.c line 50
Quoted 4 lines
block/blk-merge.c +4
-0+ split_sectors -= tmp; + +return bio_split (bio, split_sectors,GFP_NOIO , bs); +}This code to stop the discard at the previous aligned sector could be the reason why I have 2 device-mapper-test-suite tests in the 'thin-provisioning' testsuite failing due to this patchset:
Quoted 1 line
I'm setting up the testsuite to debug.
Quoted 8 lines
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_devicedmtest run --suite thin-provisioning -n
discard_fully_provisioned_device_benchmarkAgain, these tests pass without this patchset.
(trim CC list)
I followed
https://github.com/jthornber/device-mapper-test-suite
https://github.com/jthornber/thin-provisioning-toolsto setup.
$ grep CONFIG_DM_ .config CONFIG_DM_MQ_DEFAULT=y CONFIG_DM_DEBUG=y CONFIG_DM_BUFIO=y CONFIG_DM_BIO_PRISON=y CONFIG_DM_PERSISTENT_DATA=y # CONFIG_DM_DEBUG_BLOCK_STACK_TRACING is not set CONFIG_DM_CRYPT=y CONFIG_DM_SNAPSHOT=y CONFIG_DM_THIN_PROVISIONING=y CONFIG_DM_CACHE=y CONFIG_DM_CACHE_MQ=y CONFIG_DM_CACHE_SMQ=y CONFIG_DM_CACHE_CLEANER=y CONFIG_DM_ERA=y CONFIG_DM_MIRROR=y CONFIG_DM_LOG_USERSPACE=y CONFIG_DM_RAID=y CONFIG_DM_ZERO=y CONFIG_DM_MULTIPATH=y CONFIG_DM_MULTIPATH_QL=y CONFIG_DM_MULTIPATH_ST=y CONFIG_DM_DELAY=y CONFIG_DM_UEVENT=y CONFIG_DM_FLAKEY=y CONFIG_DM_VERITY=y CONFIG_DM_SWITCH=y CONFIG_DM_LOG_WRITES=y
Quoted 5 lines
# cat .dmtest/config profile :ssd do metadata_dev '/dev/vda' data_dev '/dev/vdb' end
OK, virtio-blk doesn't support discard.
I could reproduce it with virtio-scsi.
Now debugging ...
Quoted 125 lines · Mike Snitzer
Quoted 120 lines · Ming Lin
Quoted 110 lines · Mike Snitzer
Quoted 63 lines · Ming Lin
From: Kent Overstreet <[email protected]>
The way the block layer is currently written, it goes to great lengths to avoid having to split bios; upper layer code (such as
bio_add_page()) checks what the underlying device can handle and tries to always create bios that don't need to be split.But this approach becomes unwieldy and eventually breaks down with stacked devices and devices with dynamic limits, and it adds a lot of complexity. If the block layer could split bios as needed, we could eliminate a lot of complexity elsewhere - particularly in stacked drivers. Code that creates bios can then create whatever size bios are convenient, and more importantly stacked drivers don't have to deal with both their own bio size limitations and the limitations of the (potentially multiple) devices underneath them. In the future this will let us delete
merge_bvec_fnand a bunch of other code.We do this by adding calls to
blk_queue_split()to the variousmake_requestfunctions that need it - a few can already handle arbitrary size bios. Note that we add the call _after_ any call toblk_queue_bounce(); this means thatblk_queue_split()andblk_recalc_rq_segments()don't need to be concerned with bouncing affecting segment merging.Some
make_request_fn()callbacks were simple enough to audit and verify they don't needblk_queue_split()calls. The skipped ones are:*
nfhd_make_request(arch/m68k/emu/nfblock.c)
*axon_ram_make_request(arch/powerpc/sysdev/axonram.c)
*simdisk_make_request(arch/xtensa/platforms/iss/simdisk.c)
*brd_make_request(ramdisk -drivers/block/brd.c)
*mtip_submit_request(drivers/block/mtip32xx/mtip32xx.c)
*loop_make_request
*null_queue_bio
* bcache'smake_requestfnsSome others are almost certainly safe to remove now, but will be left for future patches.
Cc Jens Axboe <axboe-tSWWG44O7X1aa/[email protected]>Cc Christoph Hellwig <[email protected]>Cc Al Viro <viro-RmSDqhL/[email protected]>Cc Ming Lei <[email protected]>Cc Neil Brown <[email protected]>Cc Alasdair Kergon <[email protected]>Cc Mike Snitzer <[email protected]>Cc Lars Ellenberg <[email protected]>Cc Jiri Kosina <[email protected]>Cc Geoff Levand <[email protected]>Cc Jim Paris <jim-XrPbb/[email protected]>Cc Joshua Morris <josh.h.morris-r/[email protected]>Cc Philip Kelleher <[email protected]>Cc Minchan Kim <[email protected]>Cc Nitin Gupta <[email protected]>Cc Oleg Drokin <[email protected]>Cc Andreas Dilger <[email protected]>Signed-off-by Kent Overstreet <[email protected]>[dpark: skip more mq-based drivers, resolve merge conflicts, etc.]Signed-off-by Dongsu Park <dpark-VwIFZPTo/[email protected]>Signed-off-by Ming Lin <[email protected]>...
Quoted 43 lines
block/blk-merge.c +35
-0@@ -9,12 +9,158 @@#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)Quoted 8 lines
block/blk-merge.c line 50
Quoted 4 lines
block/blk-merge.c +4
-0+ split_sectors -= tmp; + +return bio_split (bio, split_sectors,GFP_NOIO , bs); +}This code to stop the discard at the previous aligned sector could be the reason why I have 2 device-mapper-test-suite tests in the 'thin-provisioning' testsuite failing due to this patchset:
Quoted 1 line
I'm setting up the testsuite to debug.
Quoted 8 lines
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_devicedmtest run --suite thin-provisioning -n
discard_fully_provisioned_device_benchmarkAgain, these tests pass without this patchset.
It's caused by patch 4.
When discard size >=4G, the bio->bi_iter.bi_size overflows. 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. Please see
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/commit/?h=block-generic-req&id=dcc5d9c41
I have updated both patch 1 & 4 on my tree.
commit 9607f737de9c4ca1a81655c320a61c287bf77bf5
Author: Ming Lin <[email protected]> Date: Fri May 22 00:46:56 2015 -0700
block: remove split code in blkdev_issue_discard
The split code in blkdev_issue_discard() can go away now that any driver that cares does the split, all we have to do is make sure bio size doesn't overflow.
--- block/blk-lib.c | 16 +++------------- 1 file changed, 3 insertions(+), 13 deletions(-)
block/blk-lib.c +3 -13
@@ -43,7 +43,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,DECLARE_COMPLETION_ONSTACK (wait);struct request_queue *q =bdev_get_queue (bdev);int type =REQ_WRITE |REQ_DISCARD ; -unsigned int max_discard_sectors, granularity; +unsigned int granularity;int alignment;struct bio_batch bb;struct bio *bio;
@@ -60,17 +60,6 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector, 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 ); - 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))return -EOPNOTSUPP ;
@@ -92,7 +81,8 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - req_sects =min_t (sector_t, nr_sects, max_discard_sectors); +/* Make sure bi_size doesn't overflow */ + req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 );/* * If splitting a request, and the next starting sector would be
Quoted 5 lines · Ming Lin
Quoted 4 lines · Mike Snitzer
OK, once setup, to run the 2 tests in question directly you'd do something like:
Quoted 8 lines
Quoted 5 lines
dmtest run --suite thin-provisioning -n
discard_a_fragmented_devicedmtest run --suite thin-provisioning -n
discard_fully_provisioned_device_benchmarkAgain, these tests pass without this patchset.
It's caused by patch 4.
When discard size >=4G, thebio->bi_iter.bi_sizeoverflows.
Thanks for tracking this down!
Quoted 7 lines
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 inblk_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).
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.
Mike
Quoted 8 lines · Mike Snitzer
Quoted 5 lines · Ming Lin
Quoted 4 lines · Mike Snitzer
OK, once setup, to run the 2 tests in question directly you'd do something like:
Quoted 7 lines
Quoted 7 lines
Quoted 5 lines
dmtest run --suite thin-provisioning -n
discard_a_fragmented_devicedmtest run --suite thin-provisioning -n
discard_fully_provisioned_device_benchmarkAgain, these tests pass without this patchset.
It's caused by patch 4.
Typo. I mean patch 5.
Quoted 3 lines
Quoted 1 line
When discard size >=4G, the
bio->bi_iter.bi_sizeoverflows.Thanks for tracking this down!
blkdev_issue_write_same() has same problem.
Quoted 16 lines
Quoted 8 lines
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 inblk_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_limitsbeing stacked properly. Your solution to mix discard granularity/alignment checks at the upper layer(s) but then split based onmax_discard_sectorsat the lower layer defeats that benefit for discards.
Quoted 5 lines
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
block/blk-merge.c +2 -57
@@ -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);
Quoted 3 lines
Also, it is unfortunate that IO that doesn't have a payload is being artificially split simply because
bio->bi_iter.bi_sizeis 32bits.
Indeed.
Will it be possible to make it 64bits? I guess no.
Quoted 2 lines
Mike
Quoted 2 lines · Ming Lin
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.
Quoted 11 lines · Christoph Hellwig
Quoted 2 lines · Ming Lin
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 sectorsUINT_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.
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.
Quoted 4 lines · Christoph Hellwig
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 <[email protected]> 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.
--- block/blk-lib.c | 37 +++++++++---------------------------- 1 file changed, 9 insertions(+), 28 deletions(-)
block/blk-lib.c +9 -28
@@ -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 ;
Quoted 4 lines
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.
Quoted 24 lines · Ming Lin
Quoted 4 lines · Christoph Hellwig
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 sectorsdm-thinp block size = default discard granularity = 128 sectors
blkdev_issue_discard(sector=0, nr_sectors=8388608)1. Only ensure
bi_sizenot overflowIt doesn't work.
[
start_sector,end_sector]
[0, 8388607]
Quoted 7 lines
[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 problem2. Ensure
bi_sizenot 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?
Quoted 31 lines · Kent Overstreet
Quoted 30 lines · Ming Lin
Quoted 4 lines · Christoph Hellwig
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 sectorsdm-thinp block size = default discard granularity = 128 sectors
blkdev_issue_discard(sector=0, nr_sectors=8388608)1. Only ensure
bi_sizenot overflowIt 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
Quoted 6 lines
Quoted 1 line
2. Ensure
bi_sizenot overflow and max discard size is of proper granularityIdeally 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.
Quoted 9 lines · Ming Lin
On Fri, Aug 7, 2015 at 5:30 PM, Kent Overstreet
Quoted 4 lines
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 =
0x400000sectors.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...
Quoted 2 lines · Kent Overstreet
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.
Quoted 133 lines · Ming Lin
Quoted 4 lines · Christoph Hellwig
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 sectorsdm-thinp block size = default discard granularity = 128 sectors
blkdev_issue_discard(sector=0, nr_sectors=8388608)1. Only ensure
bi_sizenot overflowIt 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 problem2. Ensure
bi_sizenot overflow and max discard size is of proper granularityIt works.
[
start_sector,end_sector]
[0, 8388607][0, 8388479] [8388480, 8388607]So how about below patch?
commit
1ca2ad977255efb3c339f4ca16fb798ed5ec54f7Author: Ming Lin <[email protected]> Date: Fri Aug 7 15:07:07 2015 -0700block: 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_sectorsis 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 <[email protected]>--- block/blk-lib.c | 37 +++++++++---------------------------- 1 file changed, 9 insertions(+), 28 deletions(-)block/blk-lib.c +7
-24@@ -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 ;
block/blk-lib.c int blkdev_issue_write_same(struct block_device *bdev, sector_t sector, · line 156
Quoted 10 lines
block/blk-lib.c +2
-4- 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
Signature · 4 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +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)
Quoted 4 lines · Hannes Reinecke
Wouldn't it be easier to move both
max_write_same_sectorsandmax_discardsectors to 64 bit (ie to typesector_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...
Quoted 10 lines · Kent Overstreet
Quoted 4 lines · Hannes Reinecke
Wouldn't it be easier to move both
max_write_same_sectorsandmax_discardsectors to 64 bit (ie to typesector_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_sizethat would have to be increased to 64 bits. Which I suppose wouldn't actually increase the size of struct bio (whensector_tis 64 bits), since structbvec_iterhas 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
Signature · 5 lines
Dr. Hannes Reinecke zSeries & Storage [email protected] +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)
Quoted 8 lines · Christoph Hellwig
Quoted 2 lines · Ming Lin
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?
Quoted 1 line · Kent Overstreet
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).
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:
Mike> This will translate to all intermediate layers that might split Mike> discards needing to worry about granularity/alignment too Mike> (e.g. how dm-thinp will have to care because it must generate Mike> discard mappings with associated bios based on how blocks were Mike> mapped to thinp).
The fundamental issue here is that alignment and granularity should never, ever have been enforced at the top of the stack. Horrendous idea from the very beginning.
For the < handful of braindead devices that get confused when you do partial or misaligned blocks we should have had a quirk that did any range adjusting at the bottom in sd_setup_discard_cmnd().
There's a reason I turned discard_zeroes_data off for UNMAP!
Wrt. the range size I don't have a problem with capping at the 32-bit bi_size limit. We probably don't want to send commands much bigger than that anyway.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 16 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:
Mike> This will translate to all intermediate layers that might split Mike> discards needing to worry about granularity/alignment too Mike> (e.g. how dm-thinp will have to care because it must generate Mike> discard mappings with associated bios based on how blocks were Mike> mapped to thinp).
The fundamental issue here is that alignment and granularity should never, ever have been enforced at the top of the stack. Horrendous idea from the very beginning.
For the < handful of braindead devices that get confused when you do partial or misaligned blocks we should have had a quirk that did any range adjusting at the bottom in
sd_setup_discard_cmnd().
Quoted 5 lines
There's a reason I turned
discard_zeroes_dataoff for UNMAP!Wrt. the range size I don't have a problem with capping at the 32-bit
bi_sizelimit. We probably don't want to send commands much bigger than that anyway.
How about below?
commit b8ca440bd77653d4d2bac90b7fd1599e9e0e150a
Author: Ming Lin <[email protected]> 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 set max discard sectors to (1<<31)>>9 to ensure it doesn't overflow bi_size and hopefully it is of the proper granularity as long as the granularity is a power of two.
--- block/blk-lib.c | 47 +++++++++++------------------------------------ 1 file changed, 11 insertions(+), 36 deletions(-)
block/blk-lib.c +11 -36
@@ -26,6 +26,13 @@ static void bio_batch_end_io(struct bio *bio, int err)bio_put (bio); } +/* +* Ensure that max discard sectors doesn't overflow bi_size and hopefully +* it is of the proper granularity as long as the granularity is a power +* of two. +*/ +#define MAX_DISCARD_SECTORS ((1U << 31) >> 9) +/** * blkdev_issue_discard - queue a discard * @bdev: blockdev to issue discard for
@@ -43,8 +50,6 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,DECLARE_COMPLETION_ONSTACK (wait);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 ;
@@ -56,21 +61,6 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,if (!blk_queue_discard (q))return -EOPNOTSUPP ; -/* 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 ); - 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))return -EOPNOTSUPP ;
@@ -84,7 +74,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) {
@@ -92,21 +82,8 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - 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. -*/ + req_sects =min_t (sector_t, nr_sects,MAX_DISCARD_SECTORS ); 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 +143,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 ;
block/blk-lib.c static void bio_batch_end_io(struct bio *bio, int err) · line 34
Quoted 6 lines · Ming Lin
block/blk-lib.c +6
-0+/* +* Ensure that max discard sectors doesn't overflow bi_size and hopefully +* it is of the proper granularity as long as the granularity is a power +* of two. +*/ +#define MAX_DISCARD_SECTORS ((1U << 31) >> 9)
Thisn't isn't MAX_DISCARD_SECTORS, it's MAX_BIO_SECTORS. If we ever to something like Kent's multipage biovecs we'll actually need it for regular read/write bios in addition to discard and write same.
Except for that the patch looks reasonable to me.
Quoted 12 lines · Christoph Hellwig
block/blk-lib.c static void bio_batch_end_io(struct bio *bio, int err) · line 34
Quoted 6 lines · Ming Lin
block/blk-lib.c +6
-0+/* +* Ensure that max discard sectors doesn't overflow bi_size and hopefully +* it is of the proper granularity as long as the granularity is a power +* of two. +*/ +#define MAX_DISCARD_SECTORS ((1U << 31) >> 9)Thisn't isn't
MAX_DISCARD_SECTORS, it'sMAX_BIO_SECTORS. If we ever to something like Kent's multipage biovecs we'll actually need it for regular read/write bios in addition to discard and write same.
Quoted 1 line
Except for that the patch looks reasonable to me.
Will change it to MAX_BIO_SECTORS.
May I add your ACK?
Quoted 2 lines · Ming Lin
Will change it to
MAX_BIO_SECTORS.
May I add your ACK?
Yes, please go ahead.
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Ming Lin
Will change it to
MAX_BIO_SECTORS.
May I add your ACK?Yes, please go ahead.
Thanks. I'll send a new version of the series once device-mapper guy acks.
Hi Mike,
I have updated my tree. Could you pull and re-test?
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-req
The 2 thin-provisioning tests passed.
Hope I can have your ACK soon.
Thanks.
Mike Snitzer
Acked-byQuoted 10 lines · Ming Lin
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Ming Lin
Will change it to
MAX_BIO_SECTORS.
May I add your ACK?Yes, please go ahead.
Thanks. I'll send a new version of the series once device-mapper guy acks.
Quoted 6 lines
Hi Mike,
I have updated my tree. Could you pull and re-test?
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-reqThe 2 thin-provisioning tests passed.
I've merged your latest branch with my dm-4.3 branch, I had one conflict in the merge due to the dm_merge_bvec() change from 4.2-rc6 (no surprise). I've published the result here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=block-late-bio-splitting
It passes the device-mapper-test-suite's 'thin-provisioning' tests.
Quoted 1 line
Hope I can have your ACK soon.
Shouldn't we also be using MAX_BIO_SECTORS in blkdev_issue_write_same (instead of UINT_MAX >> 9)?
Aside from that, I'm in favor of seeing this late bio splitting patchset finally land upstream (hopefully in time for the 4.3 merge, Jens?):
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 25 lines · Mike Snitzer
Quoted 10 lines · Ming Lin
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Ming Lin
Will change it to
MAX_BIO_SECTORS.
May I add your ACK?Yes, please go ahead.
Thanks. I'll send a new version of the series once device-mapper guy acks.
Quoted 6 lines
Hi Mike,
I have updated my tree. Could you pull and re-test?
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-reqThe 2 thin-provisioning tests passed.
I've merged your latest branch with my dm-4.3 branch, I had one conflict in the merge due to the
dm_merge_bvec()change from 4.2-rc6 (no surprise). I've published the result here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=block-late-bio-splitting
Quoted 6 lines
It passes the device-mapper-test-suite's 'thin-provisioning' tests.
Quoted 1 line
Hope I can have your ACK soon.
Shouldn't we also be using
MAX_BIO_SECTORSinblkdev_issue_write_same(instead ofUINT_MAX>> 9)?
I also prefer using MAX_BIO_SECTORS. Otherwise, we may have non page size aligned splits.
Say, write_same 8G.
Using UINT_MAX >> 9, we'll have 2 sector aligned splits in blkdev_issue_write_same():
0 - (4G - 512 - 1)
(4G - 512, 8G -1)
This looks weired.
Using MAX_BIO_SECTORS, we'll have 4 page size aligned splits:
0 - (2G -1)
2G - (4G - 1)
4G - (6G - 1)
6G - (8G - 1)
I'll use MAX_BIO_SECTORS in blkdev_issue_write_same() if no objection.
Quoted 5 lines
Aside from that, I'm in favor of seeing this late bio splitting patchset finally land upstream (hopefully in time for the 4.3 merge, Jens?):
Acked-by Mike Snitzer <[email protected]>
Thanks!
May I add your Ack to "PATCH: block: kill merge_bvec_fn() completely" also?
Quoted 4 lines
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 36 lines · Ming Lin
Quoted 25 lines · Mike Snitzer
Quoted 10 lines · Ming Lin
Quoted 5 lines · Christoph Hellwig
Quoted 2 lines · Ming Lin
Will change it to
MAX_BIO_SECTORS.
May I add your ACK?Yes, please go ahead.
Thanks. I'll send a new version of the series once device-mapper guy acks.
Quoted 6 lines
Hi Mike,
I have updated my tree. Could you pull and re-test?
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/log/?h=block-generic-reqThe 2 thin-provisioning tests passed.
I've merged your latest branch with my dm-4.3 branch, I had one conflict in the merge due to the
dm_merge_bvec()change from 4.2-rc6 (no surprise). I've published the result here:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/log/?h=block-late-bio-splittingQuoted 6 lines
It passes the device-mapper-test-suite's 'thin-provisioning' tests.
Quoted 1 line
Hope I can have your ACK soon.
Shouldn't we also be using
MAX_BIO_SECTORSinblkdev_issue_write_same(instead ofUINT_MAX>> 9)?I also prefer using
MAX_BIO_SECTORS. Otherwise, we may have non page size aligned splits.
Quoted 6 lines
Say,
write_same8G.Using
UINT_MAX>> 9, we'll have 2 sector aligned splits inblkdev_issue_write_same():
0 - (4G - 512 - 1)
(4G - 512, 8G -1)
Actually, 3 sector aligned splits.
0 - (4G-512-1)
(4G-512), (8G-512-1)
(8G-512), (8G-1)
Quoted 27 lines
This looks weired.
Using
MAX_BIO_SECTORS, we'll have 4 page size aligned splits:
0 - (2G -1)
2G - (4G - 1)
4G - (6G - 1)
6G - (8G - 1)I'll use
MAX_BIO_SECTORSinblkdev_issue_write_same()if no objection.Quoted 5 lines
Aside from that, I'm in favor of seeing this late bio splitting patchset finally land upstream (hopefully in time for the 4.3 merge, Jens?):
Acked-by Mike Snitzer <[email protected]>Thanks!
May I add your Ack to "PATCH: block: kill
merge_bvec_fn()completely" also?Quoted 4 lines
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ming" == Ming Lin <[email protected]> writes:
Ming,
Ming> I also prefer using MAX_BIO_SECTORS. Otherwise, we may have non Ming> page size aligned splits.
This does not matter for write same and discard since there is only a single logical block of payload. Also, given limitations in SATA we're always issuing 2GB-32KB sized discards. Rounding those down to an even 1GB would impact performance.
I am sympathetic to wanting to issue I/Os that are aligned to powers of two. But for most devices this matters little since the additional cost is limited to misaligned head and tail blocks.
One thing that may be worth considering is switching bi_size from bytes to blocks for REQ_FS. That would give us some headroom without increasing bi_size beyond 32 bits. But I'm not entirely sure it's worth the pain.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 11 lines · Ming Lin
Quoted 5 lines · Mike Snitzer
Aside from that, I'm in favor of seeing this late bio splitting patchset finally land upstream (hopefully in time for the 4.3 merge, Jens?):
Acked-by Mike Snitzer <[email protected]>Thanks!
May I add your Ack to "PATCH: block: kill
merge_bvec_fn()completely" also?
Sure, but please fold in the removal of dm.c comments I made in this merge commit:
thanks.
Quoted 12 lines · Mike Snitzer
Quoted 9 lines · Ming Lin
Quoted 5 lines · Mike Snitzer
Aside from that, I'm in favor of seeing this late bio splitting patchset finally land upstream (hopefully in time for the 4.3 merge, Jens?):
Acked-by Mike Snitzer <[email protected]>Thanks!
Quoted 7 lines
Quoted 2 lines
May I add your Ack to "PATCH: block: kill
merge_bvec_fn()completely" also?Sure, but please fold in the removal of
dm.ccomments I made in this merge commit:
Rebased on top of latest Linus tree(4.2-rc6+).
Quoted 2 lines
thanks.
Martin K. Petersen
Reviewed-byQuoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:
Mike> Shouldn't we also be using MAX_BIO_SECTORS in Mike> blkdev_issue_write_same (instead of UINT_MAX >> 9)?
The granularity of WRITE SAME is always 1 logical block and there is no reason to round it down to the next power of two.
+/*
+ * Ensure that max discard sectors doesn't overflow bi_size and hopefully
+ * it is of the proper granularity as long as the granularity is a power
+ * of two.
+ */
+#define MAX_DISCARD_SECTORS ((1U << 31) >> 9)
+
That's fine for SATA since we're already capping at 2TB minus change. But it means we'll be capping unnecessarily on SCSI. And larger range counts are impending in SATA as well.
So this goes back to my original comment: The only place there can be a discard granularity is for SCSI UNMAP. And consequently, we should only enforce alignment and granularity when that code path is taken in sd.c.
I'm OK with Ming's patch series in general. Let's leave the discard cap at UINT_MAX and I'll twiddle the rest in SCSI.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 20 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:
Mike> Shouldn't we also be using
MAX_BIO_SECTORSin Mike>blkdev_issue_write_same(instead ofUINT_MAX>> 9)?The granularity of WRITE SAME is always 1 logical block and there is no reason to round it down to the next power of two.
(quoted diff) +7
-0+/* + * Ensure that max discard sectors doesn't overflow bi_size and hopefully + * it is of the proper granularity as long as the granularity is a power + * of two. + */ +#define MAX_DISCARD_SECTORS ((1U << 31) >> 9) +That's fine for SATA since we're already capping at 2TB minus change. But it means we'll be capping unnecessarily on SCSI. And larger range counts are impending in SATA as well.
Quoted 6 lines
So this goes back to my original comment: The only place there can be a discard granularity is for SCSI UNMAP. And consequently, we should only enforce alignment and granularity when that code path is taken in
sd.c.I'm OK with Ming's patch series in general. Let's leave the discard cap at
UINT_MAXand I'll twiddle the rest in SCSI.
Just to make sure I didn't misunderstand it.
Did you mean still use (UINT_MAX >> 9) in blkdev_issue_discard()?
req_sects = min_t(sector_t, nr_sects, UINT_MAX >> 9); instead of: req_sects = min_t(sector_t, nr_sects, MAX_DISCARD_SECTORS);
But that doesn't work for dm-thinp. See Kent's suggestion to use 1<<31.
https://www.redhat.com/archives/dm-devel/2015-August/msg00053.html
Quoted 3 lines
Reviewed-by Martin K. Petersen <[email protected]>
Thanks!
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ming" == Ming Lin <[email protected]> writes:
Ming> Did you mean still use (UINT_MAX >> 9) in blkdev_issue_discard()?
Ming> But that doesn't work for dm-thinp. See Kent's suggestion to use Ming> 1<<31.
I'm not sure why things are not working for dm-thinp. Presumably Kent's code would split the discard at a granularity boundary so why would that cause problems for dm?
In looking at this I just found out that we'll corrupt data on certain SCSI configs with the granularity enforcement in place. I'll have to conjure up a fix for that...
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 10 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ming" == Ming Lin <[email protected]> writes:
Ming> Did you mean still use (
UINT_MAX>> 9) inblkdev_issue_discard()?Ming> But that doesn't work for dm-thinp. See Kent's suggestion to use Ming> 1<<31.
I'm not sure why things are not working for dm-thinp. Presumably Kent's code would split the discard at a granularity boundary so why would that cause problems for dm?
DM-thinp processes discards internally before it passes them down (if configured to do so). If a discard is smaller than the granularity of a thinp block (whose size is configurable) or if the start and end of the discard's extent is misaligned (relative to the thinp blocks mapped to the logical extent) then the discard won't actually discard partial thinp blocks.
Quoted 14 lines · Mike Snitzer
Quoted 10 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ming" == Ming Lin <[email protected]> writes:
Ming> Did you mean still use (
UINT_MAX>> 9) inblkdev_issue_discard()?Ming> But that doesn't work for dm-thinp. See Kent's suggestion to use Ming> 1<<31.
I'm not sure why things are not working for dm-thinp. Presumably Kent's code would split the discard at a granularity boundary so why would that cause problems for dm?
Quoted 6 lines
DM-thinp processes discards internally before it passes them down (if configured to do so). If a discard is smaller than the granularity of a thinp block (whose size is configurable) or if the start and end of the discard's extent is misaligned (relative to the thinp blocks mapped to the logical extent) then the discard won't actually discard partial thinp blocks.
This kind of logic really doesn't belong in dm - if it's needed, it really belongs in bio_split() (which is supposed to work correctly for discards - so if it is needed, then bio_split() needs fixing...)
IMO though it belongs in the driver - if a discard needs to be dropped because it's too small and the hardware can't do it, that should be the driver's responsibility.
Quoted 22 lines · Kent Overstreet
Quoted 14 lines · Mike Snitzer
Quoted 10 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ming" == Ming Lin <[email protected]> writes:
Ming> Did you mean still use (
UINT_MAX>> 9) inblkdev_issue_discard()?Ming> But that doesn't work for dm-thinp. See Kent's suggestion to use Ming> 1<<31.
I'm not sure why things are not working for dm-thinp. Presumably Kent's code would split the discard at a granularity boundary so why would that cause problems for dm?
Quoted 6 lines
DM-thinp processes discards internally before it passes them down (if configured to do so). If a discard is smaller than the granularity of a thinp block (whose size is configurable) or if the start and end of the discard's extent is misaligned (relative to the thinp blocks mapped to the logical extent) then the discard won't actually discard partial thinp blocks.
Quoted 3 lines
This kind of logic really doesn't belong in dm - if it's needed, it really belongs in
bio_split()(which is supposed to work correctly for discards - so if it is needed, thenbio_split()needs fixing...)
DM thinp does advertise discard_granularity that reflects the thinp blocksize. blk_queue_split() does look like it'd do the right thing. But the splitting that DM thinp is doing is a long standing implementation (in DM core) that will need to be carefully reviewed/rewritten. We can tackle it after all this late splitting code lands.
Quoted 3 lines
IMO though it belongs in the driver - if a discard needs to be dropped because it's too small and the hardware can't do it, that should be the driver's responsibility.
This isn't about the hardware's limits. This is about the intermediate remapping/stacking driver's own space management hooking off of the discard bio too.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Kent" == Kent Overstreet <[email protected]> writes:
Kent> This kind of logic really doesn't belong in dm
Well it does in this case since the thinp personality actually provisions and unprovisions space.
But there is a difference between what dm thinp acts on for its own internal provisioning purposes and what it passes down the stack. I am really against dropping information anywhere along the path. We don't round off read/write requests either.
The queue limits were meant as hints to mkfs.* so that on-disk data structures could be laid out in an aligned and storage friendly way. I never intended for the hints to affect runtime behavior.
Kent> IMO though it belongs in the driver - if a discard needs to be Kent> dropped because it's too small and the hardware can't do it, that Kent> should be the driver's responsibility.
I agree except I really don't want to lop off anything unless the device locks up if we send it partial blocks. There was an array that had problems a while back but I believe they have been fixed.
The fundamental premise should be that we pass as comprehensive information as we can. And the device can then decide to ignore all or parts of the request. That's fundamentally how things work at the protocol level in both SCSI and SATA. I don't see any reason why the Linux I/O stack should behave in a different manner.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Martin" == Martin K Petersen <[email protected]> writes:
Martin> I agree except I really don't want to lop off anything unless Martin> the device locks up if we send it partial blocks. There was an Martin> array that had problems a while back but I believe they have Martin> been fixed.
Oh, and there are several arrays out there that have allocation units that are not powers of two. *sigh*
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 9 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Martin" == Martin K Petersen <[email protected]> writes:
Martin> I agree except I really don't want to lop off anything unless Martin> the device locks up if we send it partial blocks. There was an Martin> array that had problems a while back but I believe they have Martin> been fixed.
Oh, and there are several arrays out there that have allocation units that are not powers of two. *sigh*
Do you still agree we cap discard to 2G as an interim solution?
https://git.kernel.org/cgit/linux/kernel/git/mlin/linux.git/commit/?h=block-generic-req&id=39d2f5f
If OK, I'll send out new version asking Jens to apply.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ming" == Ming Lin <[email protected]> writes:
Ming> Do you still agree we cap discard to 2G as an interim solution?
I can live with the 2G cap for 4.3 but would like to see it fixed properly in 4.4.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 6 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Ming" == Ming Lin <[email protected]> writes:
Ming> Do you still agree we cap discard to 2G as an interim solution?
I can live with the 2G cap for 4.3 but would like to see it fixed properly in 4.4.
Sure. I'd like to work on the fix.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:
Mike> DM-thinp processes discards internally before it passes them down Mike> (if configured to do so). If a discard is smaller than the Mike> granularity of a thinp block (whose size is configurable) or if Mike> the start and end of the discard's extent is misaligned (relative Mike> to the thinp blocks mapped to the logical extent) then the discard Mike> won't actually discard partial thinp blocks.
That's fine. You can throw away anything you don't like as long as discard_zeroes_data=0.
But I don't understand why having an artificial cap at 2GB fixes things. Other than making it less likely for you to receive a runt by virtue of being aligned to a power of two.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 11 lines · Martin K. Petersen
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:
Mike> DM-thinp processes discards internally before it passes them down Mike> (if configured to do so). If a discard is smaller than the Mike> granularity of a thinp block (whose size is configurable) or if Mike> the start and end of the discard's extent is misaligned (relative Mike> to the thinp blocks mapped to the logical extent) then the discard Mike> won't actually discard partial thinp blocks.
That's fine. You can throw away anything you don't like as long as discard_zeroes_data=0.
Correct, dm-thinp sets discard_zeroes_data=0
Quoted 3 lines
But I don't understand why having an artificial cap at 2GB fixes things. Other than making it less likely for you to receive a runt by virtue of being aligned to a power of two.
That is the benefit. And when coupled with the new default max_discard of 64K (pending change from Jens for 4.3) this 2GB upper limit really isn't such a big deal. Unless I'm missing something...
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Mike" == Mike Snitzer <[email protected]> writes:
Mike> That is the benefit. And when coupled with the new default Mike> max_discard of 64K (pending change from Jens for 4.3) this 2GB Mike> upper limit really isn't such a big deal. Unless I'm missing Mike> something...
2GB is fine for current SATA due to the stupid range descriptors. But there are some changes in the pipeline to support descriptors with bigger ranges so that will change.
However, we currently do 4GB discards on SCSI so the proposed cap will cut that in half. I'm OK with that as an interim solution. though.
But I'm a bit concerned about what might be lurking in dm thinp if you trip over partial blocks like in Ming's example...
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?
Otherwise, I can work on it.
Thanks,
Ming
Quoted 9 lines · Ming Lin
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?
Seems dm-stripe discard also needs rewrite.
Quoted 5 lines
Otherwise, I can work on it.
Thanks,
Ming
Quoted 6 lines · Ming Lin
Quoted 5 lines · Ming Lin
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 6 lines
Quoted 4 lines
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?Seems dm-stripe discard also needs rewrite.
Can you elaborate on what you feel needs re-writing in these targets?
This is the basic initial cleanup I had in mind for dm-thinp:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=dm-4.4&id=cb0aca0a6bfad6b7f7146dde776f374082a73db6
A much more involved refactoring of the dm-cache and dm-thinp targets to eliminate the need for splitting will involve bio-prison range locking and a new metadata format for both targets to express ranges as opposed to blocks. This line of work is on Joe's radar but it is much further out given the associated on-disk metadata format change.
That aside, I do need to look at DM core to see how we can do things differently so that block core's bio_split() et al is doing the splitting rather than DM core having a role.
I'd prefer to be the one working these DM changes. But if you have ideas of how things should be cleaned up I'd be happy to consider them.
Thanks,
Mike
Quoted 9 lines · Mike Snitzer
Quoted 6 lines · Ming Lin
Quoted 5 lines · Ming Lin
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?Seems dm-stripe discard also needs rewrite.
Can you elaborate on what you feel needs re-writing in these targets?
dm-stripe also require discard size to be a multiple of chunk size. See output of below debug patch for 4G discard.
root@bee:~# blkdiscard -o 0 -l 4294967296 /dev/striped_vol_group/striped_logical_volume
root@bee:~# dmesg |grep DEBUG [ 13.110224] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.113723] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.117098] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.120424] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.123800] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.127027] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.130161] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes
block/blk-lib.c +1 -1
@@ -82,7 +82,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - req_sects =min_t (sector_t, nr_sects,MAX_BIO_SECTORS ); + req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); end_sect = sector + req_sects; bio->bi_iter.bi_sector = sector;
drivers/md/dm-stripe.c +2 -0
@@ -273,6 +273,8 @@ static int stripe_map_range(struct stripe_c *sc, struct bio *bio,return DM_MAPIO_REMAPPED ; }else {/* The range doesn't map to the target stripe */ +printk ("DEBUG: discard ignored: stripe chunk size %dK bytes, bio size %d bytes\n" , + sc->chunk_size>>1 , bio->bi_iter.bi_size);bio_endio (bio);return DM_MAPIO_SUBMITTED ; }
Quoted 19 lines
This is the basic initial cleanup I had in mind for dm-thinp:
http://git.kernel.org/cgit/linux/kernel/git/snitzer/linux.git/commit/?h=dm-4.4&id=cb0aca0a6bfad6b7f7146dde776f374082a73db6A much more involved refactoring of the dm-cache and dm-thinp targets to eliminate the need for splitting will involve bio-prison range locking and a new metadata format for both targets to express ranges as opposed to blocks. This line of work is on Joe's radar but it is much further out given the associated on-disk metadata format change.
That aside, I do need to look at DM core to see how we can do things differently so that block core's
bio_split()et al is doing the splitting rather than DM core having a role.I'd prefer to be the one working these DM changes. But if you have ideas of how things should be cleaned up I'd be happy to consider them.
Thanks,
Mike
Quoted 51 lines · Ming Lin
Quoted 9 lines · Mike Snitzer
Quoted 6 lines · Ming Lin
Quoted 5 lines · Ming Lin
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?Seems dm-stripe discard also needs rewrite.
Can you elaborate on what you feel needs re-writing in these targets?
dm-stripe also require discard size to be a multiple of chunk size. See output of below debug patch for 4G discard.
root@bee:~# blkdiscard -o 0 -l 4294967296 /dev/striped_vol_group/striped_logical_volumeroot@bee:~# dmesg |grep DEBUG [ 13.110224] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.113723] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.117098] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.120424] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.123800] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.127027] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.130161] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytesblock/blk-lib.c +1
-1@@ -82,7 +82,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - req_sects =min_t (sector_t, nr_sects,MAX_BIO_SECTORS ); + req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); end_sect = sector + req_sects; bio->bi_iter.bi_sector = sector;drivers/md/dm-stripe.c +0
-0
drivers/md/dm-stripe.c static int stripe_map_range(struct stripe_c *sc, struct bio *bio, · line 280
Quoted 8 lines
drivers/md/dm-stripe.c +2
-0@@ -273,6 +273,8 @@ static int stripe_map_range(struct stripe_c *sc, struct bio *bio,return DM_MAPIO_REMAPPED ; }else {/* The range doesn't map to the target stripe */ +printk ("DEBUG: discard ignored: stripe chunk size %dK bytes, bio size %d bytes\n" , + sc->chunk_size>>1 , bio->bi_iter.bi_size);bio_endio (bio);return DM_MAPIO_SUBMITTED ; }
This is expected. If a discard is only 512 bytes and the chunk size is 128K then every discard will only ever hit one stripe.
So each discard will have N - 1 "discard ignored" messages (when N is # of stripes in the dm-stripe device). So in your test device I'd assume you have 8 stripes.
Basically your debugging looks like it is _very_ prone to false positives here. The dm-stripe code is working as expected.
Quoted 57 lines · Mike Snitzer
Quoted 54 lines · Ming Lin
Quoted 9 lines · Mike Snitzer
Quoted 6 lines · Ming Lin
Quoted 5 lines · Ming Lin
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?Seems dm-stripe discard also needs rewrite.
Can you elaborate on what you feel needs re-writing in these targets?
dm-stripe also require discard size to be a multiple of chunk size. See output of below debug patch for 4G discard.
root@bee:~# blkdiscard -o 0 -l 4294967296 /dev/striped_vol_group/striped_logical_volumeroot@bee:~# dmesg |grep DEBUG [ 13.110224] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.113723] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.117098] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.120424] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.123800] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.127027] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.130161] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytesblock/blk-lib.c +1
-1@@ -82,7 +82,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - req_sects =min_t (sector_t, nr_sects,MAX_BIO_SECTORS ); + req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); end_sect = sector + req_sects; bio->bi_iter.bi_sector = sector;drivers/md/dm-stripe.c +0
-0@@ -273,6 +273,8 @@ static int stripe_map_range(struct stripe_c *sc, struct bio *bio,return DM_MAPIO_REMAPPED ; }else {/* The range doesn't map to the target stripe */
Quoted 8 lines
drivers/md/dm-stripe.c static int stripe_map_range(struct stripe_c *sc, struct bio *bio, · line 280
Quoted 5 lines
drivers/md/dm-stripe.c +2
-0+printk ("DEBUG: discard ignored: stripe chunk size %dK bytes, bio size %d bytes\n" , + sc->chunk_size>>1 , bio->bi_iter.bi_size);bio_endio (bio);return DM_MAPIO_SUBMITTED ; }This is expected. If a discard is only 512 bytes and the chunk size is 128K then every discard will only ever hit one stripe.
The discard was actually 4G bytes.
# blkdiscard -o 0 \
-l 4294967296 /dev/striped_vol_group/striped_logical_volumeIn the above debug patch, I changed MAX_BIO_SECTORS to UINT_MAX>>9 to show the problem.
The 512 bytes comes from blkdev_issue_discard() split the 4G bytes to (UINT_MAX>>9) sectors + 1 sector.
Quoted 4 lines
So each discard will have N - 1 "discard ignored" messages (when N is # of stripes in the dm-stripe device). So in your test device I'd assume you have 8 stripes.
Yes.
Quoted 3 lines
Basically your debugging looks like it is _very_ prone to false positives here. The dm-stripe code is working as expected.
With current 2G cap in blkdev_issue_discard(), dm-stripe works OK. But if in future we change it to UINT_MAX, then dm-stripe discard will have problem as dm-thinp/dm-cache.
Quoted 85 lines · Ming Lin
Quoted 57 lines · Mike Snitzer
Quoted 54 lines · Ming Lin
Quoted 9 lines · Mike Snitzer
Quoted 6 lines · Ming Lin
Quoted 5 lines · Ming Lin
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?Seems dm-stripe discard also needs rewrite.
Can you elaborate on what you feel needs re-writing in these targets?
dm-stripe also require discard size to be a multiple of chunk size. See output of below debug patch for 4G discard.
root@bee:~# blkdiscard -o 0 -l 4294967296 /dev/striped_vol_group/striped_logical_volumeroot@bee:~# dmesg |grep DEBUG [ 13.110224] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.113723] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.117098] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.120424] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.123800] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.127027] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.130161] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytesblock/blk-lib.c +1
-1@@ -82,7 +82,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - req_sects =min_t (sector_t, nr_sects,MAX_BIO_SECTORS ); + req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); end_sect = sector + req_sects; bio->bi_iter.bi_sector = sector;drivers/md/dm-stripe.c +0
-0@@ -273,6 +273,8 @@ static int stripe_map_range(struct stripe_c *sc, struct bio *bio,return DM_MAPIO_REMAPPED ; }else {/* The range doesn't map to the target stripe */ Quoted 8 lines
drivers/md/dm-stripe.c static int stripe_map_range(struct stripe_c *sc, struct bio *bio, · line 280
Quoted 5 lines
drivers/md/dm-stripe.c +2
-0+printk ("DEBUG: discard ignored: stripe chunk size %dK bytes, bio size %d bytes\n" , + sc->chunk_size>>1 , bio->bi_iter.bi_size);bio_endio (bio);return DM_MAPIO_SUBMITTED ; }This is expected. If a discard is only 512 bytes and the chunk size is 128K then every discard will only ever hit one stripe.
The discard was actually 4G bytes.
# blkdiscard -o 0 \ -l 4294967296 /dev/striped_vol_group/striped_logical_volumeIn the above debug patch, I changed
MAX_BIO_SECTORSto UINT_MAX>>9 to show the problem.The 512 bytes comes from
blkdev_issue_discard()split the 4G bytes to (UINT_MAX>>9) sectors + 1 sector.Quoted 4 lines
So each discard will have N - 1 "discard ignored" messages (when N is # of stripes in the dm-stripe device). So in your test device I'd assume you have 8 stripes.
Yes.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
Basically your debugging looks like it is _very_ prone to false positives here. The dm-stripe code is working as expected.
With current 2G cap in
blkdev_issue_discard(), dm-stripe works OK. But if in future we change it toUINT_MAX, then dm-stripe discard will have problem as dm-thinp/dm-cache.
No you're still missing my point. dm-stripe isn't dropping the partial discard completely. It is just that the discard only applies to one of the 8 stripes in your test.
With the 2G cap it just so happens that each discard hits each stripe.
Quoted 95 lines · Mike Snitzer
Quoted 85 lines · Ming Lin
Quoted 57 lines · Mike Snitzer
Quoted 54 lines · Ming Lin
Quoted 9 lines · Mike Snitzer
Quoted 6 lines · Ming Lin
Quoted 5 lines · Ming Lin
Quoted 3 lines · Mike Snitzer
p.s. I'll be working with Joe Thornber on optimizing DM (particularly dm-thinp and dm-cache) once this patchset is included upstream. You'll see I've already added a couple WIP dm-thinp patches ontop.
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
Hi Mike,
Just to avoid duplicated work.
Are you going to work on the dm-thinp/dm-cache discard rewritten?Seems dm-stripe discard also needs rewrite.
Can you elaborate on what you feel needs re-writing in these targets?
dm-stripe also require discard size to be a multiple of chunk size. See output of below debug patch for 4G discard.
root@bee:~# blkdiscard -o 0 -l 4294967296 /dev/striped_vol_group/striped_logical_volumeroot@bee:~# dmesg |grep DEBUG [ 13.110224] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.113723] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.117098] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.120424] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.123800] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.127027] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytes [ 13.130161] DEBUG: discard ignored: stripe chunk size 128K bytes, bio size 512 bytesblock/blk-lib.c +1
-1@@ -82,7 +82,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - req_sects =min_t (sector_t, nr_sects,MAX_BIO_SECTORS ); + req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); end_sect = sector + req_sects; bio->bi_iter.bi_sector = sector;drivers/md/dm-stripe.c +0
-0@@ -273,6 +273,8 @@ static int stripe_map_range(struct stripe_c *sc, struct bio *bio,return DM_MAPIO_REMAPPED ; }else {/* The range doesn't map to the target stripe */ Quoted 8 lines
drivers/md/dm-stripe.c static int stripe_map_range(struct stripe_c *sc, struct bio *bio, · line 280
Quoted 5 lines
drivers/md/dm-stripe.c +2
-0+printk ("DEBUG: discard ignored: stripe chunk size %dK bytes, bio size %d bytes\n" , + sc->chunk_size>>1 , bio->bi_iter.bi_size);bio_endio (bio);return DM_MAPIO_SUBMITTED ; }This is expected. If a discard is only 512 bytes and the chunk size is 128K then every discard will only ever hit one stripe.
The discard was actually 4G bytes.
# blkdiscard -o 0 \ -l 4294967296 /dev/striped_vol_group/striped_logical_volumeIn the above debug patch, I changed
MAX_BIO_SECTORSto UINT_MAX>>9 to show the problem.The 512 bytes comes from
blkdev_issue_discard()split the 4G bytes to (UINT_MAX>>9) sectors + 1 sector.Quoted 4 lines
So each discard will have N - 1 "discard ignored" messages (when N is # of stripes in the dm-stripe device). So in your test device I'd assume you have 8 stripes.
Yes.
Quoted 1 line
Quoted 6 lines
Quoted 2 lines
Basically your debugging looks like it is _very_ prone to false positives here. The dm-stripe code is working as expected.
With current 2G cap in
blkdev_issue_discard(), dm-stripe works OK. But if in future we change it toUINT_MAX, then dm-stripe discard will have problem as dm-thinp/dm-cache.
Quoted 5 lines
No you're still missing my point. dm-stripe isn't dropping the partial discard completely. It is just that the discard only applies to one of the 8 stripes in your test.
With the 2G cap it just so happens that each discard hits each stripe.
Ah, yes, not I got it. dm-strip works as expected with UINT_MAX cap. I should debug with below patch.
block/blk-lib.c +1 -1
@@ -82,7 +82,7 @@ int blkdev_issue_discard(struct block_device *bdev, sector_t sector,break ; } - req_sects =min_t (sector_t, nr_sects,MAX_BIO_SECTORS ); + req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); end_sect = sector + req_sects; bio->bi_iter.bi_sector = sector;
drivers/md/dm-stripe.c +3 -0
@@ -270,9 +270,12 @@ static int stripe_map_range(struct stripe_c *sc, struct bio *bio, bio->bi_iter.bi_sector = begin + sc->stripe[target_stripe].physical_start; bio->bi_iter.bi_size =to_bytes (end - begin); +printk ("DEBUG: stripe %d, discard sector [%ld, %ld]\n" , target_stripe, + bio->bi_iter.bi_sector, (bio->bi_iter.bi_sector + (bio->bi_iter.bi_size>>9 ) -1 ));return DM_MAPIO_REMAPPED ; }else {/* The range doesn't map to the target stripe */ +printk ("DEBUG: stripe %d, discard ignored\n" , target_stripe);bio_endio (bio);return DM_MAPIO_SUBMITTED ; }
[ 17.471687] DEBUG: stripe 0, discard sector [2048, 264191] [ 17.473380] DEBUG: stripe 1, discard sector [2048, 264191] [ 17.474919] DEBUG: stripe 2, discard sector [2048, 264191] [ 17.476457] DEBUG: stripe 3, discard sector [2048, 264191] [ 17.478032] DEBUG: stripe 4, discard sector [2048, 264191] [ 17.479551] DEBUG: stripe 5, discard sector [2048, 264191] [ 17.481064] DEBUG: stripe 6, discard sector [2048, 264191] [ 17.482607] DEBUG: stripe 7, discard sector [2048, 264191]
[ 17.484136] DEBUG: stripe 0, discard sector [264192, 526335] [ 17.485669] DEBUG: stripe 1, discard sector [264192, 526335] [ 17.487226] DEBUG: stripe 2, discard sector [264192, 526335] [ 17.488778] DEBUG: stripe 3, discard sector [264192, 526335] [ 17.490388] DEBUG: stripe 4, discard sector [264192, 526335] [ 17.491998] DEBUG: stripe 5, discard sector [264192, 526335] [ 17.493613] DEBUG: stripe 6, discard sector [264192, 526335] [ 17.495199] DEBUG: stripe 7, discard sector [264192, 526335]
[ 17.496725] DEBUG: stripe 0, discard sector [526336, 788479] [ 17.498168] DEBUG: stripe 1, discard sector [526336, 788479] [ 17.499574] DEBUG: stripe 2, discard sector [526336, 788479] [ 17.500902] DEBUG: stripe 3, discard sector [526336, 788479] [ 17.502341] DEBUG: stripe 4, discard sector [526336, 788479] [ 17.503734] DEBUG: stripe 5, discard sector [526336, 788479] [ 17.505347] DEBUG: stripe 6, discard sector [526336, 788479] [ 17.506984] DEBUG: stripe 7, discard sector [526336, 788479]
[ 17.508485] DEBUG: stripe 0, discard sector [788480, 1050623] [ 17.509998] DEBUG: stripe 1, discard sector [788480, 1050623] [ 17.511567] DEBUG: stripe 2, discard sector [788480, 1050623] [ 17.513118] DEBUG: stripe 3, discard sector [788480, 1050623] [ 17.514665] DEBUG: stripe 4, discard sector [788480, 1050623] [ 17.516246] DEBUG: stripe 5, discard sector [788480, 1050623] [ 17.517830] DEBUG: stripe 6, discard sector [788480, 1050623] [ 17.519398] DEBUG: stripe 7, discard sector [788480, 1050622]
[ 17.520802] DEBUG: stripe 0, discard ignored [ 17.521953] DEBUG: stripe 1, discard ignored [ 17.523167] DEBUG: stripe 2, discard ignored [ 17.524321] DEBUG: stripe 3, discard ignored [ 17.525544] DEBUG: stripe 4, discard ignored [ 17.526737] DEBUG: stripe 5, discard ignored [ 17.527876] DEBUG: stripe 6, discard ignored [ 17.529024] DEBUG: stripe 7, discard sector [1050623, 1050623]