Add throttling for discards
Jens Axboe
maintainerv2 cover Add throttling for discards
I implemented support for discards in blk-wbt, so we treat them like background writes. If we have competing foreground IO, then we may throttle discards. Otherwise they should run at full speed.
Ran a quick local test case on a NVMe device. The test case here is doing reads from a file, while other files are being delete. File system is XFS. Three separate runs:
1) 'justreader' is just running the reader workload by itself.
2) 'nothrottle' is running the reader with the unlinks on the stock kernel.
3) 'throttled' is running the reader with the unlinks with the patches applied.
The fio run logs latencies in windows of 50msec, logging only the worst latency seen in that window:
[read]
filename=file0
rw=randread
bs=4k
direct=1
ioengine=libaio
iodepth=8
write_lat_log=read_lat
log_avg_msec=50
log_max_value=1
Graph of the latencies here: http://kernel.dk/wbt-discard.jpg
As you can see, with the throttling, the latencies and performance is the same as not having the unlinks running. They finish within 100msec of each other, after having read 10G of data. Without the throttling and unlinks running, we see spikes that are almost an order of magnitude worse. The reader also takes 1 second longer to complete. 322MB/sec vs 313MB/sec. It's also visible in the higher Pxx. Here's the throttled end of the percentiles:
| 99.00th=[ 145], 99.50th=[ 153], 99.90th=[ 180], 99.95th=[ 198],
| 99.99th=[ 889]and here's the unthrottled equivalent:
| 99.00th=[ 145], 99.50th=[ 155], 99.90th=[ 188], 99.95th=[ 322],
| 99.99th=[ 8586]Changes since v1:
- Pass in enum wbt_flags for get_rq_wait()
- Turn wbt_should_throttle() into a switch statement
blk-lib.c | 7 +++--- blk-stat.h | 6 ++--- blk-wbt.c | 70 ++++++++++++++++++++++++++++++++++++------------------------- blk-wbt.h | 8 +++++- 4 files changed, 55 insertions(+), 36 deletions(-)
Jens Axboe
maintainerPATCH v2 1/4 block: break discard submissions into the user defined size
Don't build discards bigger than what the user asked for, if the user decided to limit the size by writing to 'discard_max_bytes'.
--- block/blk-lib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
block/blk-lib.c +4 -3
@@ -62,10 +62,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,unsigned int req_sects;sector_t end_sect, tmp; -/* Make sure bi_size doesn't overflow */ - req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); +/* Issue in chunks of the user defined max discard setting */ + req_sects =min_t (sector_t, nr_sects, + q->limits.max_discard_sectors); -/** +/* * If splitting a request, and the next starting sector would be * misaligned, stop the discard at the previous aligned sector. */
Signature · 1 line
2.7.4
Re: [PATCH 1/4] block: break discard submissions into the user defined size
Quoted 4 lines · Jens Axboe
Don't build discards bigger than what the user asked for, if the user decided to limit the size by writing to '
discard_max_bytes'.Signed-off-by Jens Axboe <[email protected]>
Seems fine to me,
--D
Quoted 30 lines
--- block/blk-lib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)block/blk-lib.c +4
-3@@ -62,10 +62,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,unsigned int req_sects;sector_t end_sect, tmp; -/* Make sure bi_size doesn't overflow */ - req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); +/* Issue in chunks of the user defined max discard setting */ + req_sects =min_t (sector_t, nr_sects, + q->limits.max_discard_sectors); -/** +/* * If splitting a request, and the next starting sector would be * misaligned, stop the discard at the previous aligned sector. */ Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] block: break discard submissions into the user defined size
In reply to Jens AxboeQuoted 13 lines · Jens Axboe
Don't build discards bigger than what the user asked for, if the user decided to limit the size by writing to '
discard_max_bytes'.Signed-off-by Jens Axboe <[email protected]>--- block/blk-lib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)block/blk-lib.c +0
-0
block/blk-lib.c int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, · line 68
Quoted 9 lines
block/blk-lib.c +3
-2@@ -62,10 +62,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,unsigned int req_sects;sector_t end_sect, tmp; -/* Make sure bi_size doesn't overflow */ - req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); +/* Issue in chunks of the user defined max discard setting */ + req_sects =min_t (sector_t, nr_sects, + q->limits.max_discard_sectors);
Some drivers, including nvme, do
blk_queue_max_discard_sectors(q, UINT_MAX)
That means q->limits.max_discard_sectors can be UINT_MAX, and
bio->bi_iter.bi_size = req_sects << 9;
will overflow. We should probably cap max_discard_sectors at UINT_MAX >> 9 in a prep patch.
Jens Axboe
maintainerRe: [PATCH 1/4] block: break discard submissions into the user defined size
Quoted 26 lines · Omar Sandoval
Quoted 13 lines · Jens Axboe
Don't build discards bigger than what the user asked for, if the user decided to limit the size by writing to '
discard_max_bytes'.Signed-off-by Jens Axboe <[email protected]>--- block/blk-lib.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)block/blk-lib.c +0
-0block/blk-lib.c int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, · line 68
Quoted 9 lines
block/blk-lib.c +3
-2@@ -62,10 +62,11 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector,unsigned int req_sects;sector_t end_sect, tmp; -/* Make sure bi_size doesn't overflow */ - req_sects =min_t (sector_t, nr_sects,UINT_MAX >>9 ); +/* Issue in chunks of the user defined max discard setting */ + req_sects =min_t (sector_t, nr_sects, + q->limits.max_discard_sectors);Some drivers, including nvme, do
Quoted 8 lines
blk_queue_max_discard_sectors(q, UINT_MAX)That means
q->limits.max_discard_sectorscan beUINT_MAX, andbio->bi_iter.bi_size = req_sects << 9;will overflow. We should probably cap
max_discard_sectorsatUINT_MAX>> 9 in a prep patch.
Probably better to just keep the local overflow check, I'll do that. Thanks for spotting it!
Signature · 1 line
Jens Axboe
Jens Axboe
maintainerPATCH v2 2/4 blk-wbt: account any writing command as a write
In reply to Jens AxboeWe currently special case WRITE and FLUSH, but we should really just include any command with the write bit set. This ensures that we account DISCARD.
--- block/blk-wbt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
block/blk-wbt.c +1 -1
@@ -701,7 +701,7 @@ static int wbt_data_dir(const struct request *rq)if (op ==REQ_OP_READ )return READ ; -else if (op ==REQ_OP_WRITE || op ==REQ_OP_FLUSH ) +else if (op_is_write (op))return WRITE ;/* don't account */
Signature · 1 line
2.7.4
Re: [PATCH 2/4] blk-wbt: account any writing command as a write
Quoted 6 lines · Jens Axboe
We currently special case WRITE and FLUSH, but we should really just include any command with the write bit set. This ensures that we account DISCARD.
Reviewed-by Christoph Hellwig <[email protected]>Signed-off-by Jens Axboe <[email protected]>
Looks ok,
--D
Quoted 24 lines
--- block/blk-wbt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)block/blk-wbt.c +1
-1@@ -701,7 +701,7 @@ static int wbt_data_dir(const struct request *rq)if (op ==REQ_OP_READ )return READ ; -else if (op ==REQ_OP_WRITE || op ==REQ_OP_FLUSH ) +else if (op_is_write (op))return WRITE ;/* don't account */ Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] blk-wbt: account any writing command as a write
In reply to Jens AxboeQuoted 5 lines · Jens Axboe
We currently special case WRITE and FLUSH, but we should really just include any command with the write bit set. This ensures that we account DISCARD.
Reviewed-by Christoph Hellwig <[email protected]>
Quoted 25 lines
Signed-off-by Jens Axboe <[email protected]>--- block/blk-wbt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)block/blk-wbt.c +1
-1@@ -701,7 +701,7 @@ static int wbt_data_dir(const struct request *rq)if (op ==REQ_OP_READ )return READ ; -else if (op ==REQ_OP_WRITE || op ==REQ_OP_FLUSH ) +else if (op_is_write (op))return WRITE ;/* don't account */ Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Jens Axboe
maintainerPATCH v2 3/4 blk-wbt: pass in enum wbt_flags to get_rq_wait()
In reply to Jens AxboeThis is in preparation for having more write queues, in which case we would have needed to pass in more information than just a simple 'is_kswapd' boolean.
--- block/blk-wbt.c | 25 +++++++++++++++---------- block/blk-wbt.h | 4 +++- 2 files changed, 18 insertions(+), 11 deletions(-)
block/blk-wbt.c +15 -10
@@ -101,9 +101,13 @@ static bool wb_recent_wait(struct rq_wb *rwb)return time_before (jiffies, wb->dirty_sleep +HZ ); } -static inline struct rq_wait *get_rq_wait (struct rq_wb *rwb,bool is_kswapd) +static inline struct rq_wait *get_rq_wait (struct rq_wb *rwb, +enum wbt_flags wb_acct) { -return &rwb->rq_wait[is_kswapd]; +if (wb_acct &WBT_KSWAPD ) +return &rwb->rq_wait[WBT_RWQ_KSWAPD ]; + +return &rwb->rq_wait[WBT_RWQ_BG ]; }static void rwb_wake_all (struct rq_wb *rwb)
@@ -126,7 +130,7 @@ void __wbt_done(struct rq_wb *rwb, enum wbt_flags wb_acct)if (!(wb_acct &WBT_TRACKED ))return ; - rqw =get_rq_wait (rwb, wb_acct &WBT_KSWAPD ); + rqw =get_rq_wait (rwb, wb_acct); inflight =atomic_dec_return (&rqw->inflight);/*
@@ -529,11 +533,12 @@ static inline bool may_queue(struct rq_wb *rwb, struct rq_wait *rqw,* Block if we will exceed our limit, or if we are currently waiting for * the timer to kick off queuing again. */ -static void __wbt_wait (struct rq_wb *rwb,unsigned long rw,spinlock_t *lock) +static void __wbt_wait (struct rq_wb *rwb,enum wbt_flags wb_acct, +unsigned long rw,spinlock_t *lock)__releases (lock)__acquires (lock) { -struct rq_wait *rqw =get_rq_wait (rwb,current_is_kswapd ()); +struct rq_wait *rqw =get_rq_wait (rwb, wb_acct);DEFINE_WAIT (wait);if (may_queue (rwb, rqw, &wait, rw))
@@ -584,7 +589,7 @@ static inline bool wbt_should_throttle(struct rq_wb *rwb, struct bio *bio)*/ enum wbt_flags wbt_wait (struct rq_wb *rwb,struct bio *bio,spinlock_t *lock) { -unsigned int ret =0 ; +enum wbt_flags ret =0 ;if (!rwb_enabled (rwb))return 0 ;
@@ -598,14 +603,14 @@ enum wbt_flags wbt_wait(struct rq_wb *rwb, struct bio *bio, spinlock_t *lock)return ret; } -__wbt_wait (rwb, bio->bi_opf, lock); +if (current_is_kswapd ()) + ret |=WBT_KSWAPD ; + +__wbt_wait (rwb, ret, bio->bi_opf, lock);if (!blk_stat_is_active (rwb->cb))rwb_arm_timer (rwb); -if (current_is_kswapd ()) - ret |=WBT_KSWAPD ; -return ret |WBT_TRACKED ; }
block/blk-wbt.h +3 -1
@@ -19,7 +19,9 @@ enum wbt_flags { };enum { -WBT_NUM_RWQ =2 , +WBT_RWQ_BG =0 , +WBT_RWQ_KSWAPD , +WBT_NUM_RWQ , };/*
Signature · 1 line
2.7.4
Re: [PATCH 3/4] blk-wbt: pass in enum wbt_flags to get_rq_wait()
Quoted 5 lines · Jens Axboe
This is in preparation for having more write queues, in which case we would have needed to pass in more information than just a simple '
is_kswapd' boolean.Signed-off-by Jens Axboe <[email protected]>
Looks ok (having not run any kind of testing on this yet),
--D
Quoted 99 lines
--- block/blk-wbt.c | 25 +++++++++++++++---------- block/blk-wbt.h | 4 +++- 2 files changed, 18 insertions(+), 11 deletions(-)block/blk-wbt.c +15
-10@@ -101,9 +101,13 @@ static bool wb_recent_wait(struct rq_wb *rwb)return time_before (jiffies, wb->dirty_sleep +HZ ); } -static inline struct rq_wait *get_rq_wait (struct rq_wb *rwb,bool is_kswapd) +static inline struct rq_wait *get_rq_wait (struct rq_wb *rwb, +enum wbt_flags wb_acct) { -return &rwb->rq_wait[is_kswapd]; +if (wb_acct &WBT_KSWAPD ) +return &rwb->rq_wait[WBT_RWQ_KSWAPD ]; + +return &rwb->rq_wait[WBT_RWQ_BG ]; }static void rwb_wake_all (struct rq_wb *rwb)@@ -126,7 +130,7 @@ void __wbt_done(struct rq_wb *rwb, enum wbt_flags wb_acct)if (!(wb_acct &WBT_TRACKED ))return ; - rqw =get_rq_wait (rwb, wb_acct &WBT_KSWAPD ); + rqw =get_rq_wait (rwb, wb_acct); inflight =atomic_dec_return (&rqw->inflight);/* @@ -529,11 +533,12 @@ static inline bool may_queue(struct rq_wb *rwb, struct rq_wait *rqw,* Block if we will exceed our limit, or if we are currently waiting for * the timer to kick off queuing again. */ -static void __wbt_wait (struct rq_wb *rwb,unsigned long rw,spinlock_t *lock) +static void __wbt_wait (struct rq_wb *rwb,enum wbt_flags wb_acct, +unsigned long rw,spinlock_t *lock)__releases (lock)__acquires (lock) { -struct rq_wait *rqw =get_rq_wait (rwb,current_is_kswapd ()); +struct rq_wait *rqw =get_rq_wait (rwb, wb_acct);DEFINE_WAIT (wait);if (may_queue (rwb, rqw, &wait, rw))@@ -584,7 +589,7 @@ static inline bool wbt_should_throttle(struct rq_wb *rwb, struct bio *bio)*/ enum wbt_flags wbt_wait (struct rq_wb *rwb,struct bio *bio,spinlock_t *lock) { -unsigned int ret =0 ; +enum wbt_flags ret =0 ;if (!rwb_enabled (rwb))return 0 ;@@ -598,14 +603,14 @@ enum wbt_flags wbt_wait(struct rq_wb *rwb, struct bio *bio, spinlock_t *lock)return ret; } -__wbt_wait (rwb, bio->bi_opf, lock); +if (current_is_kswapd ()) + ret |=WBT_KSWAPD ; + +__wbt_wait (rwb, ret, bio->bi_opf, lock);if (!blk_stat_is_active (rwb->cb))rwb_arm_timer (rwb); -if (current_is_kswapd ()) - ret |=WBT_KSWAPD ; -return ret |WBT_TRACKED ; }block/blk-wbt.h +3
-1@@ -19,7 +19,9 @@ enum wbt_flags { };enum { -WBT_NUM_RWQ =2 , +WBT_RWQ_BG =0 , +WBT_RWQ_KSWAPD , +WBT_NUM_RWQ , };/* Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] blk-wbt: pass in enum wbt_flags to get_rq_wait()
In reply to Jens AxboeQuoted 3 lines · Jens Axboe
This is in preparation for having more write queues, in which case we would have needed to pass in more information than just a simple '
is_kswapd' boolean.
Quoted 6 lines
Signed-off-by Jens Axboe <[email protected]>--- block/blk-wbt.c | 25 +++++++++++++++---------- block/blk-wbt.h | 4 +++- 2 files changed, 18 insertions(+), 11 deletions(-)
Jens Axboe
maintainerPATCH v2 4/4 blk-wbt: throttle discards like background writes
In reply to Jens AxboeThrottle discards like we would any background write. Discards should be background activity, so if they are impacting foreground IO, then we will throttle them down.
--- block/blk-stat.h | 6 +++--- block/blk-wbt.c | 43 ++++++++++++++++++++++++++----------------- block/blk-wbt.h | 4 +++- 3 files changed, 32 insertions(+), 21 deletions(-)
block/blk-stat.h +3 -3
@@ -10,11 +10,11 @@/* * from upper: -* 3 bits: reserved for other usage +* 4 bits: reserved for other usage * 12 bits: size -* 49 bits: time +* 48 bits: time */ -#define BLK_STAT_RES_BITS 3 +#define BLK_STAT_RES_BITS 4#define BLK_STAT_SIZE_BITS 12#define BLK_STAT_RES_SHIFT (64 - BLK_STAT_RES_BITS)#define BLK_STAT_SIZE_SHIFT (BLK_STAT_RES_SHIFT - BLK_STAT_SIZE_BITS)
block/blk-wbt.c +26 -17
75 lines not on this page · raw message
block/blk-wbt.h +3 -1
@@ -14,13 +14,15 @@ enum wbt_flags {WBT_TRACKED =1 ,/* write, tracked for throttling */ WBT_READ =2 ,/* read */ WBT_KSWAPD =4 ,/* write, from kswapd */ +WBT_DISCARD =8 ,/* discard */ - WBT_NR_BITS = 3, /* number of bits */ + WBT_NR_BITS = 4, /* number of bits */ };enum {WBT_RWQ_BG =0 ,WBT_RWQ_KSWAPD , +WBT_RWQ_DISCARD ,WBT_NUM_RWQ , };
Signature · 1 line
2.7.4
Re: [PATCH 4/4] blk-wbt: throttle discards like background writes
Quoted 5 lines · Jens Axboe
Throttle discards like we would any background write. Discards should be background activity, so if they are impacting foreground IO, then we will throttle them down.
Signed-off-by Jens Axboe <[email protected]>
Looks ok,
--D
Quoted 137 lines
--- block/blk-stat.h | 6 +++--- block/blk-wbt.c | 43 ++++++++++++++++++++++++++----------------- block/blk-wbt.h | 4 +++- 3 files changed, 32 insertions(+), 21 deletions(-)block/blk-stat.h +3
-3@@ -10,11 +10,11 @@/* * from upper: -* 3 bits: reserved for other usage +* 4 bits: reserved for other usage * 12 bits: size -* 49 bits: time +* 48 bits: time */ -#define BLK_STAT_RES_BITS 3 +#define BLK_STAT_RES_BITS 4#define BLK_STAT_SIZE_BITS 12#define BLK_STAT_RES_SHIFT (64 - BLK_STAT_RES_BITS)#define BLK_STAT_SIZE_SHIFT (BLK_STAT_RES_SHIFT - BLK_STAT_SIZE_BITS)block/blk-wbt.c +26
-17@@ -106,6 +106,8 @@ static inline struct rq_wait *get_rq_wait(struct rq_wb *rwb, {if (wb_acct &WBT_KSWAPD )return &rwb->rq_wait[WBT_RWQ_KSWAPD ]; +else if (wb_acct &WBT_DISCARD ) +return &rwb->rq_wait[WBT_RWQ_DISCARD ];return &rwb->rq_wait[WBT_RWQ_BG ]; }@@ -143,10 +145,13 @@ void __wbt_done(struct rq_wb *rwb, enum wbt_flags wb_acct) }/* -* If the device does write back caching, drop further down -* before we wake people up. +* For discards, our limit is always the background. For writes, if +* the device does write back caching, drop further down before we +* wake people up. */ -if (rwb->wc && !wb_recent_wait (rwb)) +if (wb_acct &WBT_DISCARD ) + limit = rwb->wb_background; +else if (rwb->wc && !wb_recent_wait (rwb)) limit =0 ;else limit = rwb->wb_normal;@@ -483,6 +488,9 @@ static inline unsigned int get_limit(struct rq_wb *rwb, unsigned long rw) {unsigned int limit; +if ((rw &REQ_OP_MASK ) ==REQ_OP_DISCARD ) +return rwb->wb_background; +/* * At this point we know it's a buffered write. If this is * kswapd trying to free memory, or REQ_SYNC is set, then @@ -564,21 +572,20 @@ static void __wbt_wait(struct rq_wb *rwb, enum wbt_flags wb_acct,static inline bool wbt_should_throttle (struct rq_wb *rwb,struct bio *bio) { -const int op =bio_op (bio); - -/* -* If not a WRITE, do nothing -*/ -if (op !=REQ_OP_WRITE ) -return false; - -/* -* Don't throttle WRITE_ODIRECT -*/ -if ((bio->bi_opf & (REQ_SYNC |REQ_IDLE )) == (REQ_SYNC |REQ_IDLE )) +switch (bio_op (bio)) { +case REQ_OP_WRITE : +/* +* Don't throttle WRITE_ODIRECT +*/ +if ((bio->bi_opf & (REQ_SYNC |REQ_IDLE )) == + (REQ_SYNC |REQ_IDLE )) +return false; +/* fallthrough */ +case REQ_OP_DISCARD : +return true; +default :return false; - -return true; + } }/* @@ -605,6 +612,8 @@ enum wbt_flags wbt_wait(struct rq_wb *rwb, struct bio *bio, spinlock_t *lock)if (current_is_kswapd ()) ret |=WBT_KSWAPD ; +if (bio_op (bio) ==REQ_OP_DISCARD ) + ret |=WBT_DISCARD ;__wbt_wait (rwb, ret, bio->bi_opf, lock);block/blk-wbt.h +3
-1@@ -14,13 +14,15 @@ enum wbt_flags {WBT_TRACKED =1 ,/* write, tracked for throttling */ WBT_READ =2 ,/* read */ WBT_KSWAPD =4 ,/* write, from kswapd */ +WBT_DISCARD =8 ,/* discard */ - WBT_NR_BITS = 3, /* number of bits */ + WBT_NR_BITS = 4, /* number of bits */ };enum {WBT_RWQ_BG =0 ,WBT_RWQ_KSWAPD , +WBT_RWQ_DISCARD ,WBT_NUM_RWQ , };Signature · 6 lines
2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] blk-wbt: throttle discards like background writes
In reply to Jens AxboeQuoted 3 lines · Jens Axboe
Throttle discards like we would any background write. Discards should be background activity, so if they are impacting foreground IO, then we will throttle them down.
Seems reasonable.
Quoted 6 lines
Signed-off-by Jens Axboe <[email protected]>--- block/blk-stat.h | 6 +++--- block/blk-wbt.c | 43 ++++++++++++++++++++++++++----------------- block/blk-wbt.h | 4 +++- 3 files changed, 32 insertions(+), 21 deletions(-)