Message ID | 20200713123511.19441-3-colyli@suse.de (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | two generic block layer fixes for 5.9 | expand |
Hi Coly! > This patch improves discard bio split for address and size alignment > in __blkdev_issue_discard(). The aligned discard bio may help > underlying device controller to perform better discard and internal > garbage collection, and avoid unnecessary internal fragment. If the aim is to guarantee that all discard requests, except for head and tail, are aligned multiples of the discard_granularity, you also need to take the discard_alignment queue limit and the partition offset into consideration.
On 2020/7/14 00:47, Martin K. Petersen wrote: > > Hi Coly! > >> This patch improves discard bio split for address and size alignment >> in __blkdev_issue_discard(). The aligned discard bio may help >> underlying device controller to perform better discard and internal >> garbage collection, and avoid unnecessary internal fragment. > Hi Martin, > If the aim is to guarantee that all discard requests, except for head > and tail, are aligned multiples of the discard_granularity, you also > need to take the discard_alignment queue limit and the partition offset > into consideration. > The discard_granularity was considered and my though is, discard_alignment normally is multiples of discard granularity, if the discard bio bi_sector and bi_size are aligned to discard granularity, when underlying driver splits its discard bio by discard_alignment, the split bio bi_sector and bi_size can still be aligned to discard granularity. Another reason I don't handle discard_alignment alignment in __blkdev_issue_discard() is performance. Handling discard_alignment bio split in driver's split loop may call blk_next_bio() much less in __blkdev_issue_discard(), and be more friendly for memory and cache footprint. For the partition offset, my original idea was to suggest the partition or dm target starts on offset 2048 sectors. But your opinion sound better, if considering partition or target offset maybe users misconfigured the partition offset may also gain the benefit of discard alignment. Let me try to improve a v3 patch to handle the partition offset too. Thanks for the cool idea :-) Coly Li
diff --git a/block/blk-lib.c b/block/blk-lib.c index 5f2c429d4378..7bffdee63a20 100644 --- a/block/blk-lib.c +++ b/block/blk-lib.c @@ -55,8 +55,29 @@ int __blkdev_issue_discard(struct block_device *bdev, sector_t sector, return -EINVAL; while (nr_sects) { - sector_t req_sects = min_t(sector_t, nr_sects, - bio_allowed_max_sectors(q)); + sector_t granularity_aligned_lba; + sector_t req_sects; + + granularity_aligned_lba = round_up(sector, + q->limits.discard_granularity >> SECTOR_SHIFT); + + /* + * Check whether the discard bio starts at a discard_granularity + * aligned LBA, + * - If no: set (granularity_aligned_lba - sector) to bi_size of + * the first split bio, then the second bio will start at a + * discard_granularity aligned LBA. + * - If yes: use bio_aligned_discard_max_sectors() as the max + * possible bi_size of the first split bio. Then when this bio + * is split in device drive, the split ones are very probably + * to be aligned to discard_granularity of the device's queue. + */ + if (granularity_aligned_lba == sector) + req_sects = min_t(sector_t, nr_sects, + bio_aligned_discard_max_sectors(q)); + else + req_sects = min_t(sector_t, nr_sects, + granularity_aligned_lba - sector); WARN_ON_ONCE((req_sects << 9) > UINT_MAX); diff --git a/block/blk.h b/block/blk.h index b5d1f0fc6547..a80738581f84 100644 --- a/block/blk.h +++ b/block/blk.h @@ -281,6 +281,20 @@ static inline unsigned int bio_allowed_max_sectors(struct request_queue *q) return round_down(UINT_MAX, queue_logical_block_size(q)) >> 9; } +/* + * The max bio size which is aligned to q->limits.discard_granularity. This + * is a hint to split large discard bio in generic block layer, then if device + * driver needs to split the discard bio into smaller ones, their bi_size can + * be very probably and easily aligned to discard_granularity of the device's + * queue. + */ +static inline unsigned int bio_aligned_discard_max_sectors( + struct request_queue *q) +{ + return round_down(UINT_MAX, q->limits.discard_granularity) >> + SECTOR_SHIFT; +} + /* * Internal io_context interface */