Improve zoned (SMR) HDD write throughput
Subsystems (2)
Damien Le Moal
maintainercover Improve zoned (SMR) HDD write throughput
Jens,
This patch series cleans up the zone write plugging code and introduces the ability to issue all write BIOs from a single context (a kthread) instead of allowing multiple zones to be written at the same time using a per zone work. As shown in patch 6, raw block device tests and XFS tests with an SMR HDD show that this can significantly increase write throughput (up to 40% over the current zone write plugging).
Damien Le Moal (8):
block: fix zone write plug removal
block: remove BLK_ZONE_WPLUG_UNHASHED
block: remove disk_zone_is_full()
block: improve disk_zone_wplug_schedule_bio_work()
block: rename struct gendisk zone_wplugs_lock field
block: allow submitting all zone writes from a single context
block: default to QD=1 writes for blk-mq rotational zoned devices
Documentation: ABI: stable: document the zoned_qd1_writes attribute
Documentation/ABI/stable/sysfs-block | 15 ++ block/blk-mq-debugfs.c | 1 + block/blk-sysfs.c | 43 ++++ block/blk-zoned.c | 312 ++++++++++++++++++++------- include/linux/blkdev.h | 10 +- 5 files changed, 298 insertions(+), 83 deletions(-)
Signature · 1 line
2.53.0
Damien Le Moal
maintainerPATCH 1/8 block: fix zone write plug removal
Commit 7b295187287e ("block: Do not remove zone write plugs still in use") modified disk_should_remove_zone_wplug() to add a check on the reference count of a zone write plug to prevent removing zone write plugs from a disk hash table when the plugs are still being referenced by BIOs in-flight. However, this check does not take into account that a BIO completion may happen right after its submission by a zone write plug BIO work, and before the zone write plug BIO work release the zone write plug reference count. This situation leads to disk_should_remove_zone_wplug() returning false. If the BIO that completes in such manner transitioned the zone to the FULL condition, the zone write plug for the FULL zone will remain in the disk hash table.
Solve this by moving the check for removing a zone write plug from its disk hash table from disk_zone_wplug_unplug_bio() into a new function disk_check_and_put_zone_wplug() so that we can always check if a zone write plug should be removed when the plug references are being released. This new function calls disk_should_remove_zone_wplug() and disk_remove_zone_wplug() with the zone write plug lock held when the zone write plug reference count is equal to the minimum value (2) indicating that only the caller context is referencing the zone write plug. This is followed with a call to disk_put_zone_wplug() which may cause the zone write plug to be freed if the plug was removed from the disk hash table and the plug reference count dropped to 0.
The simpler disk_put_zone_write_plug() function is used whenever we know that we have multiple references on a zone write plug, that is, when we know that checking for the zone removal is not necessary.
Of note is that the calls to disk_should_remove_zone_wplug() and disk_remove_zone_zone_wplug() are maintained in disk_zone_wplug_set_wp_offset() because this function is called directly while walking the disk zone write plug hash table without taking references on zone write plugs when processing zone reset all operations.
--- block/blk-zoned.c | 52 +++++++++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 18 deletions(-)
block/blk-zoned.c +34 -18
@@ -655,6 +655,29 @@ static void disk_remove_zone_wplug(struct gendisk *disk,disk_put_zone_wplug (zwplug); } +static inline void disk_check_and_put_zone_wplug (struct blk_zone_wplug *zwplug) +{ +lockdep_assert_not_held (&zwplug->lock); + +/* +* Check for the need to remove the zone write plug from the hash list +* when we see that only the caller is referencing the zone write plug. +* Since this is racy without holding the zone write plug lock, this +* check is done again in disk_should_remove_zone_wplug(). +*/ +if (refcount_read (&zwplug->ref) <=2 ) { +struct gendisk *disk = zwplug->disk; +unsigned long flags; + +spin_lock_irqsave (&zwplug->lock, flags); +if (disk_should_remove_zone_wplug (disk, zwplug)) +disk_remove_zone_wplug (disk, zwplug); +spin_unlock_irqrestore (&zwplug->lock, flags); + } + +disk_put_zone_wplug (zwplug); +} +static void blk_zone_wplug_bio_work (struct work_struct *work);/*
@@ -835,7 +858,7 @@ static unsigned int disk_zone_wplug_sync_wp_offset(struct gendisk *disk,if (zwplug->flags &BLK_ZONE_WPLUG_NEED_WP_UPDATE )disk_zone_wplug_set_wp_offset (disk, zwplug, wp_offset);spin_unlock_irqrestore (&zwplug->lock, flags); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); }return wp_offset;
@@ -1017,14 +1040,14 @@ int blkdev_get_zone_info(struct block_device *bdev, sector_t sector,spin_lock_irqsave (&zwplug->lock, flags);if (zwplug->flags &BLK_ZONE_WPLUG_NEED_WP_UPDATE ) {spin_unlock_irqrestore (&zwplug->lock, flags); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug);return blkdev_report_zone_fallback (bdev, sector, zone); } zone->cond = zwplug->cond; zone->wp = sector + zwplug->wp_offset;spin_unlock_irqrestore (&zwplug->lock, flags); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug);return 0 ; }
@@ -1107,7 +1130,7 @@ static void blk_zone_reset_bio_endio(struct bio *bio)spin_lock_irqsave (&zwplug->lock, flags);disk_zone_wplug_set_wp_offset (disk, zwplug,0 );spin_unlock_irqrestore (&zwplug->lock, flags); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); }else {disk_zone_set_cond (disk, sector,BLK_ZONE_COND_EMPTY ); }
@@ -1165,7 +1188,7 @@ static void blk_zone_finish_bio_endio(struct bio *bio)disk_zone_wplug_set_wp_offset (disk, zwplug,bdev_zone_sectors (bdev));spin_unlock_irqrestore (&zwplug->lock, flags); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); }else {disk_zone_set_cond (disk, sector,BLK_ZONE_COND_FULL ); }
@@ -1530,7 +1553,7 @@ static void blk_zone_wplug_handle_native_zone_append(struct bio *bio)disk_remove_zone_wplug (disk, zwplug);spin_unlock_irqrestore (&zwplug->lock, flags); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); }static bool blk_zone_wplug_handle_zone_mgmt (struct bio *bio)
@@ -1631,13 +1654,6 @@ static void disk_zone_wplug_unplug_bio(struct gendisk *disk, zwplug->flags &= ~BLK_ZONE_WPLUG_PLUGGED ; -/* -* If the zone is full (it was fully written or finished, or empty -* (it was reset), remove its zone write plug from the hash table. -*/ -if (disk_should_remove_zone_wplug (disk, zwplug)) -disk_remove_zone_wplug (disk, zwplug); -spin_unlock_irqrestore (&zwplug->lock, flags); }
@@ -1701,7 +1717,7 @@ void blk_zone_write_plug_bio_endio(struct bio *bio)disk_zone_wplug_unplug_bio (disk, zwplug);/* Drop the reference we took when entering this function. */ -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); }void blk_zone_write_plug_finish_request (struct request *req)
@@ -1724,7 +1740,7 @@ void blk_zone_write_plug_finish_request(struct request *req)disk_zone_wplug_unplug_bio (disk, zwplug);/* Drop the reference we took when entering this function. */ -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); }static void blk_zone_wplug_bio_work (struct work_struct *work)
@@ -1777,7 +1793,7 @@ static void blk_zone_wplug_bio_work(struct work_struct *work) put_zwplug:/* Drop the reference we took in disk_zone_wplug_schedule_bio_work(). */ -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); }void disk_init_zone_resources (struct gendisk *disk)
@@ -1850,7 +1866,7 @@ static void disk_destroy_zone_wplugs_hash_table(struct gendisk *disk) struct blk_zone_wplug, node);refcount_inc (&zwplug->ref);disk_remove_zone_wplug (disk, zwplug); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug); } }
@@ -2112,7 +2128,7 @@ static int blk_revalidate_seq_zone(struct blk_zone *zone, unsigned int idx,if (!zwplug)return -ENOMEM ;spin_unlock_irqrestore (&zwplug->lock, flags); -disk_put_zone_wplug (zwplug); +disk_check_and_put_zone_wplug (zwplug);return 0 ; }
Signature · 1 line
2.53.0
Re: [PATCH 1/8] block: fix zone write plug removal
Quoted 67 lines · Damien Le Moal
Commit
7b295187287e("block: Do not remove zone write plugs still in use") modifieddisk_should_remove_zone_wplug()to add a check on the reference count of a zone write plug to prevent removing zone write plugs from a disk hash table when the plugs are still being referenced by BIOs in-flight. However, this check does not take into account that a BIO completion may happen right after its submission by a zone write plug BIO work, and before the zone write plug BIO work release the zone write plug reference count. This situation leads todisk_should_remove_zone_wplug()returning false. If the BIO that completes in such manner transitioned the zone to the FULL condition, the zone write plug for the FULL zone will remain in the disk hash table.Solve this by moving the check for removing a zone write plug from its disk hash table from
disk_zone_wplug_unplug_bio()into a new functiondisk_check_and_put_zone_wplug()so that we can always check if a zone write plug should be removed when the plug references are being released. This new function callsdisk_should_remove_zone_wplug()anddisk_remove_zone_wplug()with the zone write plug lock held when the zone write plug reference count is equal to the minimum value (2) indicating that only the caller context is referencing the zone write plug. This is followed with a call todisk_put_zone_wplug()which may cause the zone write plug to be freed if the plug was removed from the disk hash table and the plug reference count dropped to 0.The simpler
disk_put_zone_write_plug()function is used whenever we know that we have multiple references on a zone write plug, that is, when we know that checking for the zone removal is not necessary.Of note is that the calls to
disk_should_remove_zone_wplug()anddisk_remove_zone_zone_wplug()are maintained indisk_zone_wplug_set_wp_offset()because this function is called directly while walking the disk zone write plug hash table without taking references on zone write plugs when processing zone reset all operations.Fixes 7b295187287e ("block: Do not remove zone write plugs still in use")Signed-off-by Damien Le Moal <[email protected]>--- block/blk-zoned.c | 52 +++++++++++++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 18 deletions(-)block/blk-zoned.c +16
-0@@ -655,6 +655,29 @@ static void disk_remove_zone_wplug(struct gendisk *disk,disk_put_zone_wplug (zwplug); } +static inline void disk_check_and_put_zone_wplug (struct blk_zone_wplug *zwplug) +{ +lockdep_assert_not_held (&zwplug->lock); + +/* +* Check for the need to remove the zone write plug from the hash list +* when we see that only the caller is referencing the zone write plug. +* Since this is racy without holding the zone write plug lock, this +* check is done again in disk_should_remove_zone_wplug(). +*/ +if (refcount_read (&zwplug->ref) <=2 ) { +struct gendisk *disk = zwplug->disk; +unsigned long flags; + +spin_lock_irqsave (&zwplug->lock, flags); +if (disk_should_remove_zone_wplug (disk, zwplug))
block/blk-zoned.c static void disk_remove_zone_wplug(struct gendisk *disk, · line 683
Quoted 10 lines
block/blk-zoned.c +7
-0+disk_remove_zone_wplug (disk, zwplug); +spin_unlock_irqrestore (&zwplug->lock, flags); + } + +disk_put_zone_wplug (zwplug); +} +static void blk_zone_wplug_bio_work (struct work_struct *work);/*
This looks slightly odd; a simple 'refcount_read()' always trigger alarm bells with me as it inevitably races with 'refcount_dec()' / 'refcount_inc()'.
And similar the check 'refcount <= 2' also looks odd; typically one would check against '< 1' and let the refcount destructor handle things. But anyway, that's how the code does it, so:
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Re: [PATCH 1/8] block: fix zone write plug removal
Quoted 28 lines · Hannes Reinecke
Quoted 27 lines · Damien Le Moal
+static inline void
disk_check_and_put_zone_wplug(struct blk_zone_wplug *zwplug)block/blk-zoned.c +5
-0+{ + lockdep_assert_not_held(&zwplug->lock); + + /* +* Check for the need to remove the zone write plug from the hash list
+ * when we see that only the caller is referencing the zone write plug.
+ * Since this is racy without holding the zone write plug lock, this + * check is done again indisk_should_remove_zone_wplug(). + */
+ if (refcount_read(&zwplug->ref)<= 2) {
+ struct gendisk *disk =zwplug->disk;
+ unsigned long flags;block/blk-zoned.c +10
-0+ + spin_lock_irqsave(&zwplug->lock, flags); + if (disk_should_remove_zone_wplug(disk, zwplug)) + disk_remove_zone_wplug(disk, zwplug); + spin_unlock_irqrestore(&zwplug->lock, flags); + } + + disk_put_zone_wplug(zwplug); +} +static void
blk_zone_wplug_bio_work(struct work_struct *work);
Quoted 8 lines
Quoted 1 line
/*
This looks slightly odd; a simple '
refcount_read()' always trigger alarm bells with me as it inevitably races with 'refcount_dec()' / 'refcount_inc()'.
And similar the check 'refcount <= 2' also looks odd; typically one would check against '< 1' and let the refcount destructor handle things.
Hmm ... I think that the above code is racy. Here is an example:
thread 1 thread 2 ------------------------------- ------------------------------- refcount_read() returns 3 refcount_read() returns 3
disk_should_remove_zone_wplug() is disk_should_remove_zone_wplug()
is not called is not called
disk_put_zone_wplug() decreases disk_put_zone_wplug() decreaseszwplug->ref from 3 to 2 zwplug->ref from 2 to 1
Result: zwplug->ref became less than 2 without disk_should_remove_zone_wplug() having been called.
Thanks,
Bart.
Re: [PATCH 1/8] block: fix zone write plug removal
Quoted 44 lines · Bart Van Assche
Quoted 29 lines · Hannes Reinecke
Quoted 28 lines · Damien Le Moal
+static inline void
disk_check_and_put_zone_wplug(struct blk_zone_wplug *zwplug)block/blk-zoned.c +5
-0+{ + lockdep_assert_not_held(&zwplug->lock); + + /* +* Check for the need to remove the zone write plug from the hash list
+ * when we see that only the caller is referencing the zone write plug.
+ * Since this is racy without holding the zone write plug lock, this
+ * check is done again indisk_should_remove_zone_wplug(). + */
+ if (refcount_read(&zwplug->ref)<= 2) {
+ struct gendisk *disk =zwplug->disk;
+ unsigned long flags;block/blk-zoned.c +10
-0+ + spin_lock_irqsave(&zwplug->lock, flags); + if (disk_should_remove_zone_wplug(disk, zwplug)) + disk_remove_zone_wplug(disk, zwplug); + spin_unlock_irqrestore(&zwplug->lock, flags); + } + + disk_put_zone_wplug(zwplug); +} +static void
blk_zone_wplug_bio_work(struct work_struct *work);Quoted 8 lines
Quoted 1 line
/*
This looks slightly odd; a simple '
refcount_read()' always trigger alarm bells with me as it inevitably races with 'refcount_dec()' / 'refcount_inc()'.
And similar the check 'refcount <= 2' also looks odd; typically one would check against '< 1' and let the refcount destructor handle things.Hmm ... I think that the above code is racy. Here is an example:
thread 1 thread 2 ------------------------------- -------------------------------
refcount_read()returns 3refcount_read()returns 3
Quoted 8 lines
disk_should_remove_zone_wplug()isdisk_should_remove_zone_wplug()is not called is not called
disk_put_zone_wplug()decreasesdisk_put_zone_wplug()decreaseszwplug->reffrom 3 to 2zwplug->reffrom 2 to 1Result:
zwplug->refbecame less than 2 withoutdisk_should_remove_zone_wplug()having been called.
A correction to the above: three threads are required before it becomes possible for the zwplug reference count to drop from 3 to zero without disk_should_remove_zone_wplug() having been called.
Thanks,
Bart.
Damien Le Moal
maintainerRe: [PATCH 1/8] block: fix zone write plug removal
Quoted 49 lines · Bart Van Assche
Quoted 48 lines · Bart Van Assche
Quoted 26 lines · Hannes Reinecke
Quoted 25 lines · Damien Le Moal
+static inline void
disk_check_and_put_zone_wplug(struct blk_zone_wplug *zwplug)block/blk-zoned.c +22
-0+{ + lockdep_assert_not_held(&zwplug->lock); + + /* + * Check for the need to remove the zone write plug from the hash list + * when we see that only the caller is referencing the zone write plug. + * Since this is racy without holding the zone write plug lock, this + * check is done again in disk_should_remove_zone_wplug(). + */ + if (refcount_read(&zwplug->ref) <= 2) { + struct gendisk *disk = zwplug->disk; + unsigned long flags; + + spin_lock_irqsave(&zwplug->lock, flags); + if (disk_should_remove_zone_wplug(disk, zwplug)) + disk_remove_zone_wplug(disk, zwplug); + spin_unlock_irqrestore(&zwplug->lock, flags); + } + + disk_put_zone_wplug(zwplug); +} +static void
blk_zone_wplug_bio_work(struct work_struct *work);Quoted 8 lines
Quoted 1 line
/*
This looks slightly odd; a simple '
refcount_read()' always trigger alarm bells with me as it inevitably races with 'refcount_dec()' / 'refcount_inc()'.
And similar the check 'refcount <= 2' also looks odd; typically one would check against '< 1' and let the refcount destructor handle things.Hmm ... I think that the above code is racy. Here is an example:
thread 1 thread 2 ------------------------------- -------------------------------
refcount_read()returns 3refcount_read()returns 3
disk_should_remove_zone_wplug()isdisk_should_remove_zone_wplug()is not called is not called
disk_put_zone_wplug()decreasesdisk_put_zone_wplug()decreaseszwplug->reffrom 3 to 2zwplug->reffrom 2 to 1
Quoted 6 lines
Quoted 2 lines
Result:
zwplug->refbecame less than 2 withoutdisk_should_remove_zone_wplug()having been called.A correction to the above: three threads are required before it becomes possible for the zwplug reference count to drop from 3 to zero without
disk_should_remove_zone_wplug()having been called.
I do not see how your example is possible... If refcount read are form the BIO work context, we only have one per zone, so there is not such concurrency. The additional contexts can be from zone reset or zone finish, but these unconditionally call disk_should_remove_zone_wplug(), so there is no such race.
Quoted 4 lines
Thanks,
Bart.
Signature · 2 lines
Damien Le Moal Western Digital Research
Damien Le Moal
maintainerPATCH 2/8 block: remove BLK_ZONE_WPLUG_UNHASHED
In reply to Damien Le MoalDetecting that a zone write plug has been removed from a disk hash table can be done using hlist_unhashed(), so there is no need for the zone write plug flag BLK_ZONE_WPLUG_UNHASHED. Remove this flag and convert all tests using it to use hlist_unhashed().
--- block/blk-zoned.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-)
block/blk-zoned.c +10 -20
@@ -99,17 +99,9 @@ static inline unsigned int disk_zone_wplugs_hash_size(struct gendisk *disk)* being executed or the zone write plug bio list is not empty. * - BLK_ZONE_WPLUG_NEED_WP_UPDATE: Indicates that we lost track of a zone * write pointer offset and need to update it. -* - BLK_ZONE_WPLUG_UNHASHED: Indicates that the zone write plug was removed -* from the disk hash table and that the initial reference to the zone -* write plug set when the plug was first added to the hash table has been -* dropped. This flag is set when a zone is reset, finished or become full, -* to prevent new references to the zone write plug to be taken for -* newly incoming BIOs. A zone write plug flagged with this flag will be -* freed once all remaining references from BIOs or functions are dropped. */ #define BLK_ZONE_WPLUG_PLUGGED (1U << 0)#define BLK_ZONE_WPLUG_NEED_WP_UPDATE (1U << 1) -#define BLK_ZONE_WPLUG_UNHASHED (1U << 2)/** * blk_zone_cond_str - Return a zone condition name string
@@ -592,7 +584,7 @@ static inline void disk_put_zone_wplug(struct blk_zone_wplug *zwplug)if (refcount_dec_and_test (&zwplug->ref)) {WARN_ON_ONCE (!bio_list_empty (&zwplug->bio_list));WARN_ON_ONCE (zwplug->flags &BLK_ZONE_WPLUG_PLUGGED ); -WARN_ON_ONCE (!(zwplug->flags &BLK_ZONE_WPLUG_UNHASHED )); +WARN_ON_ONCE (!hlist_unhashed_lockless (&zwplug->node));call_rcu (&zwplug->rcu_head, disk_free_zone_wplug_rcu); }
@@ -603,14 +595,14 @@ static inline bool disk_should_remove_zone_wplug(struct gendisk *disk, {lockdep_assert_held (&zwplug->lock); -/* If the zone write plug was already removed, we are done. */ -if (zwplug->flags &BLK_ZONE_WPLUG_UNHASHED ) -return false; -/* If the zone write plug is still plugged, it cannot be removed. */ if (zwplug->flags &BLK_ZONE_WPLUG_PLUGGED )return false; +/* If the zone write plug was already removed, we have nothing to do. */ +if (hlist_unhashed (&zwplug->node)) +return false; +/* * Completions of BIOs with blk_zone_write_plug_bio_endio() may * happen after handling a request completion with
@@ -635,16 +627,14 @@ static void disk_remove_zone_wplug(struct gendisk *disk,unsigned long flags;/* If the zone write plug was already removed, we have nothing to do. */ -if (zwplug->flags &BLK_ZONE_WPLUG_UNHASHED ) +if (hlist_unhashed (&zwplug->node))return ;/* -* Mark the zone write plug as unhashed and drop the extra reference we -* took when the plug was inserted in the hash table. Also update the -* disk zone condition array with the current condition of the zone -* write plug. +* Update the disk zone condition array with the current condition of +* the zone write plug and drop the extra reference we took when the +* plug was inserted in the hash table. */ - zwplug->flags |=BLK_ZONE_WPLUG_UNHASHED ;spin_lock_irqsave (&disk->zone_wplugs_lock, flags); blk_zone_set_cond(rcu_dereference_check(disk->zones_cond, lockdep_is_held(&disk->zone_wplugs_lock)),
@@ -702,7 +692,7 @@ static struct blk_zone_wplug *disk_get_and_lock_zone_wplug(struct gendisk *disk,* we need to get a new plug so start over from the beginning. */ spin_lock_irqsave (&zwplug->lock, *flags); -if (zwplug->flags &BLK_ZONE_WPLUG_UNHASHED ) { +if (hlist_unhashed (&zwplug->node)) {spin_unlock_irqrestore (&zwplug->lock, *flags);disk_put_zone_wplug (zwplug); goto again;
Signature · 1 line
2.53.0
Re: [PATCH 2/8] block: remove BLK_ZONE_WPLUG_UNHASHED
Quoted 51 lines · Damien Le Moal
Detecting that a zone write plug has been removed from a disk hash table can be done using
hlist_unhashed(), so there is no need for the zone write plug flagBLK_ZONE_WPLUG_UNHASHED. Remove this flag and convert all tests using it to usehlist_unhashed().Suggested-by Christoph Hellwig <[email protected]>Signed-off-by Damien Le Moal <[email protected]>--- block/blk-zoned.c | 30 ++++++++++-------------------- 1 file changed, 10 insertions(+), 20 deletions(-)block/blk-zoned.c +1
-13@@ -99,17 +99,9 @@ static inline unsigned int disk_zone_wplugs_hash_size(struct gendisk *disk)* being executed or the zone write plug bio list is not empty. * - BLK_ZONE_WPLUG_NEED_WP_UPDATE: Indicates that we lost track of a zone * write pointer offset and need to update it. -* - BLK_ZONE_WPLUG_UNHASHED: Indicates that the zone write plug was removed -* from the disk hash table and that the initial reference to the zone -* write plug set when the plug was first added to the hash table has been -* dropped. This flag is set when a zone is reset, finished or become full, -* to prevent new references to the zone write plug to be taken for -* newly incoming BIOs. A zone write plug flagged with this flag will be -* freed once all remaining references from BIOs or functions are dropped. */ #define BLK_ZONE_WPLUG_PLUGGED (1U << 0)#define BLK_ZONE_WPLUG_NEED_WP_UPDATE (1U << 1) -#define BLK_ZONE_WPLUG_UNHASHED (1U << 2)/** * blk_zone_cond_str - Return a zone condition name string @@ -592,7 +584,7 @@ static inline void disk_put_zone_wplug(struct blk_zone_wplug *zwplug)if (refcount_dec_and_test (&zwplug->ref)) {WARN_ON_ONCE (!bio_list_empty (&zwplug->bio_list));WARN_ON_ONCE (zwplug->flags &BLK_ZONE_WPLUG_PLUGGED ); -WARN_ON_ONCE (!(zwplug->flags &BLK_ZONE_WPLUG_UNHASHED )); +WARN_ON_ONCE (!hlist_unhashed_lockless (&zwplug->node));call_rcu (&zwplug->rcu_head, disk_free_zone_wplug_rcu); }@@ -603,14 +595,14 @@ static inline bool disk_should_remove_zone_wplug(struct gendisk *disk, {lockdep_assert_held (&zwplug->lock); -/* If the zone write plug was already removed, we are done. */ -if (zwplug->flags &BLK_ZONE_WPLUG_UNHASHED ) -return false; -/* If the zone write plug is still plugged, it cannot be removed. */
block/blk-zoned.c static inline bool disk_should_remove_zone_wplug(struct gendisk *disk, · line 608
Quoted 10 lines
block/blk-zoned.c +4
-0if (zwplug->flags &BLK_ZONE_WPLUG_PLUGGED )return false; +/* If the zone write plug was already removed, we have nothing to do. */ +if (hlist_unhashed (&zwplug->node)) +return false; +/* * Completions of BIOs with blk_zone_write_plug_bio_endio() may * happen after handling a request completion with
The order of the checks have changed here; not sure if that matters, but as it stands it's not a 1:1 conversion.
Maybe move this check up.
Quoted 11 lines
block/blk-zoned.c +1
-3@@ -635,16 +627,14 @@ static void disk_remove_zone_wplug(struct gendisk *disk,unsigned long flags;/* If the zone write plug was already removed, we have nothing to do. */ -if (zwplug->flags &BLK_ZONE_WPLUG_UNHASHED ) +if (hlist_unhashed (&zwplug->node))return ;/* -* Mark the zone write plug as unhashed and drop the extra reference we -* took when the plug was inserted in the hash table. Also update the
block/blk-zoned.c static void disk_remove_zone_wplug(struct gendisk *disk, · line 640
Quoted 10 lines
block/blk-zoned.c +3
-3-* disk zone condition array with the current condition of the zone -* write plug. +* Update the disk zone condition array with the current condition of +* the zone write plug and drop the extra reference we took when the +* plug was inserted in the hash table. */ - zwplug->flags |=BLK_ZONE_WPLUG_UNHASHED ;spin_lock_irqsave (&disk->zone_wplugs_lock, flags); blk_zone_set_cond(rcu_dereference_check(disk->zones_cond, lockdep_is_held(&disk->zone_wplugs_lock)),
Especially as the checks here at in the same order.
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Damien Le Moal
maintainerRe: [PATCH 2/8] block: remove BLK_ZONE_WPLUG_UNHASHED
Quoted 13 lines · Hannes Reinecke
Quoted 13 lines
block/blk-zoned.c +0
-0@@ -603,14 +595,14 @@ static inline bool disk_should_remove_zone_wplug(structgendisk *disk, { lockdep_assert_held(&zwplug->lock);- /* If the zone write plug was already removed, we are done. */ - if (
zwplug->flags&BLK_ZONE_WPLUG_UNHASHED)
- return false;
-
/* If the zone write plug is still plugged, it cannot be removed. */ if (zwplug->flags&BLK_ZONE_WPLUG_PLUGGED)
return false;
+ /* If the zone write plug was already removed, we have nothing to do. */ + if (hlist_unhashed(&zwplug->node))
Quoted 8 lines
Quoted 5 lines
+ return false;
+
/*
* Completions of BIOs withblk_zone_write_plug_bio_endio()may
* happen after handling a request completion withThe order of the checks have changed here; not sure if that matters, but as it stands it's not a 1:1 conversion.
Maybe move this check up.
Will do.
Signature · 2 lines
Damien Le Moal Western Digital Research
Damien Le Moal
maintainerPATCH 3/8 block: remove disk_zone_is_full()
In reply to Damien Le MoalThe helper function disk_zone_is_full() is only used in disk_zone_wplug_is_full(). So remove it and open code it directly in this single caller.
--- block/blk-zoned.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
block/blk-zoned.c +3 -9
@@ -484,18 +484,12 @@ static bool disk_zone_is_last(struct gendisk *disk, struct blk_zone *zone)return zone->start + zone->len >=get_capacity (disk); } -static bool disk_zone_is_full (struct gendisk *disk, -unsigned int zno,unsigned int offset_in_zone) -{ -if (zno < disk->nr_zones -1 ) -return offset_in_zone >= disk->zone_capacity; -return offset_in_zone >= disk->last_zone_capacity; -} -static bool disk_zone_wplug_is_full (struct gendisk *disk,struct blk_zone_wplug *zwplug) { -return disk_zone_is_full (disk, zwplug->zone_no, zwplug->wp_offset); +if (zwplug->zone_no < disk->nr_zones -1 ) +return zwplug->wp_offset >= disk->zone_capacity; +return zwplug->wp_offset >= disk->last_zone_capacity; } static bool disk_insert_zone_wplug(struct gendisk *disk,
Signature · 1 line
2.53.0
Re: [PATCH 3/8] block: remove disk_zone_is_full()
Quoted 9 lines · Damien Le Moal
The helper function
disk_zone_is_full()is only used indisk_zone_wplug_is_full(). So remove it and open code it directly in this single caller.Signed-off-by Damien Le Moal <[email protected]>--- block/blk-zoned.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Re: [PATCH 3/8] block: remove disk_zone_is_full()
In reply to Damien Le MoalLooks good,
Damien Le Moal
maintainerPATCH 4/8 block: improve disk_zone_wplug_schedule_bio_work()
In reply to Damien Le MoalThe function disk_zone_wplug_schedule_bio_work() always takes a reference on the zone write plug of the BIO work being scheduled. This ensure that the zone write plug cannot be freed while the BIO work is being scheduled but has not run yet. However, this unconditional reference taking is fragile since the reference taken is realized by the BIO work blk_zone_wplug_bio_work() function, which implies that there always must be a 1:1 relation between the work being scheduled and the work running.
Make this less fragile by taking the reference on the BIO work if and only if the BIO work has not been already scheduled.
--- block/blk-zoned.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
block/blk-zoned.c +7 -5
@@ -1205,13 +1205,15 @@ static void disk_zone_wplug_schedule_bio_work(struct gendisk *disk,lockdep_assert_held (&zwplug->lock);/* -* Take a reference on the zone write plug and schedule the submission -* of the next plugged BIO. blk_zone_wplug_bio_work() will release the -* reference we take here. +* Schedule the submission of the next plugged BIO. If the zone write +* plug BIO work is not already scheduled, take a reference on the zone +* write plug to ensure that it does not go away while the work is being +* scheduled but has not run yet. blk_zone_wplug_bio_work() will release +* the reference we take here. */ WARN_ON_ONCE (!(zwplug->flags &BLK_ZONE_WPLUG_PLUGGED )); -refcount_inc (&zwplug->ref); -queue_work (disk->zone_wplugs_wq, &zwplug->bio_work); +if (queue_work (disk->zone_wplugs_wq, &zwplug->bio_work)) +refcount_inc (&zwplug->ref); } static inline void disk_zone_wplug_add_bio(struct gendisk *disk,
Signature · 1 line
2.53.0
Re: [PATCH 4/8] block: improve disk_zone_wplug_schedule_bio_work()
Quoted 32 lines · Damien Le Moal
The function
disk_zone_wplug_schedule_bio_work()always takes a reference on the zone write plug of the BIO work being scheduled. This ensure that the zone write plug cannot be freed while the BIO work is being scheduled but has not run yet. However, this unconditional reference taking is fragile since the reference taken is realized by the BIO workblk_zone_wplug_bio_work()function, which implies that there always must be a 1:1 relation between the work being scheduled and the work running.Make this less fragile by taking the reference on the BIO work if and only if the BIO work has not been already scheduled.
Signed-off-by Damien Le Moal <[email protected]>--- block/blk-zoned.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)block/blk-zoned.c +4
-3@@ -1205,13 +1205,15 @@ static void disk_zone_wplug_schedule_bio_work(struct gendisk *disk,lockdep_assert_held (&zwplug->lock);/* -* Take a reference on the zone write plug and schedule the submission -* of the next plugged BIO. blk_zone_wplug_bio_work() will release the -* reference we take here. +* Schedule the submission of the next plugged BIO. If the zone write +* plug BIO work is not already scheduled, take a reference on the zone +* write plug to ensure that it does not go away while the work is being +* scheduled but has not run yet. blk_zone_wplug_bio_work() will release
block/blk-zoned.c static void disk_zone_wplug_schedule_bio_work(struct gendisk *disk, · line 1219
Quoted 10 lines
block/blk-zoned.c +3
-2+* the reference we take here. */ WARN_ON_ONCE (!(zwplug->flags &BLK_ZONE_WPLUG_PLUGGED )); -refcount_inc (&zwplug->ref); -queue_work (disk->zone_wplugs_wq, &zwplug->bio_work); +if (queue_work (disk->zone_wplugs_wq, &zwplug->bio_work)) +refcount_inc (&zwplug->ref); } static inline void disk_zone_wplug_add_bio(struct gendisk *disk,
Urgh. Don't.
There is a race condition; 'bio_work' might be scheduled directly and complete before 'refcount_inc()' is called.
You have to invert the statement by keeping the 'refcount_inc()' before calling 'queue_work()', and then drop the reference again if queue_work failed.
Ugly, I know.
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Re: [PATCH 4/8] block: improve disk_zone_wplug_schedule_bio_work()
Quoted 9 lines · Hannes Reinecke
Quoted 7 lines · Damien Le Moal
WARN_ON_ONCE(!(zwplug->flags & BLK_ZONE_WPLUG_PLUGGED)); -refcount_inc(&zwplug->ref);
-queue_work(disk->zone_wplugs_wq, &zwplug->bio_work); + if (queue_work(disk->zone_wplugs_wq, &zwplug->bio_work)) +refcount_inc(&zwplug->ref);
}
static inline void disk_zone_wplug_add_bio(struct gendisk *disk,
Quoted 6 lines
Urgh. Don't.
There is a race condition; 'bio_work' might be scheduled directly and complete before 'refcount_inc()' is called.
You have to invert the statement by keeping the 'refcount_inc()' before calling 'queue_work()', and then drop the reference again ifqueue_workfailed.
Hi Hannes,
I think that the above code is fine because the caller holds zwplug->lock. blk_zone_wplug_bio_work() only decrements zwplug->ref after it has locked and unlocked the same lock.
Bart.
Damien Le Moal
maintainerRe: [PATCH 4/8] block: improve disk_zone_wplug_schedule_bio_work()
In reply to Hannes ReineckeQuoted 43 lines · Hannes Reinecke
Quoted 31 lines · Damien Le Moal
The function
disk_zone_wplug_schedule_bio_work()always takes a reference on the zone write plug of the BIO work being scheduled. This ensure that the zone write plug cannot be freed while the BIO work is being scheduled but has not run yet. However, this unconditional reference taking is fragile since the reference taken is realized by the BIO workblk_zone_wplug_bio_work()function, which implies that there always must be a 1:1 relation between the work being scheduled and the work running.Make this less fragile by taking the reference on the BIO work if and only if the BIO work has not been already scheduled.
Signed-off-by Damien Le Moal <[email protected]>---
block/blk-zoned.c| 12 +++++++-----1 file changed, 7 insertions(+), 5 deletions(-)block/blk-zoned.c +3
-3@@ -1205,13 +1205,15 @@ static void disk_zone_wplug_schedule_bio_work(structgendisk *disk, lockdep_assert_held(&zwplug->lock); /* -* Take a reference on the zone write plug and schedule the submission -* of the next plugged BIO. blk_zone_wplug_bio_work() will release the -* reference we take here. +* Schedule the submission of the next plugged BIO. If the zone write +* plug BIO work is not already scheduled, take a reference on the zone +* write plug to ensure that it does not go away while the work is being block/blk-zoned.c static void disk_zone_wplug_schedule_bio_work(struct · line 1218
Quoted 10 lines
block/blk-zoned.c +4
-2+* scheduled but has not run yet. blk_zone_wplug_bio_work() will release +* the reference we take here. */ WARN_ON_ONCE(!(zwplug->flags & BLK_ZONE_WPLUG_PLUGGED)); - refcount_inc(&zwplug->ref); - queue_work(disk->zone_wplugs_wq, &zwplug->bio_work); + if (queue_work(disk->zone_wplugs_wq, &zwplug->bio_work)) + refcount_inc(&zwplug->ref); } static inline void disk_zone_wplug_add_bio(struct gendisk *disk,
Quoted 7 lines
Urgh. Don't.
There is a race condition; 'bio_work' might be scheduled directly and complete before 'refcount_inc()' is called.
You have to invert the statement by keeping the 'refcount_inc()' before calling 'queue_work()', and then drop the reference again ifqueue_workfailed.
Ugly, I know.
As Bart pointed out, since this is done with the zone write plug spinlock held and the work takes that lock first when it runs, there is no chance that the refcount gets bad. That is indeed not super clear though, so I will imrove the comment to describe that.
Note: the zone write plug refcounting is a bit a mess and needs cleaning up, which I was planning to do on top of all this.
Signature · 2 lines
Damien Le Moal Western Digital Research
Re: [PATCH 4/8] block: improve disk_zone_wplug_schedule_bio_work()
Quoted 52 lines · Damien Le Moal
Quoted 43 lines · Hannes Reinecke
Quoted 31 lines · Damien Le Moal
The function
disk_zone_wplug_schedule_bio_work()always takes a reference on the zone write plug of the BIO work being scheduled. This ensure that the zone write plug cannot be freed while the BIO work is being scheduled but has not run yet. However, this unconditional reference taking is fragile since the reference taken is realized by the BIO workblk_zone_wplug_bio_work()function, which implies that there always must be a 1:1 relation between the work being scheduled and the work running.Make this less fragile by taking the reference on the BIO work if and only if the BIO work has not been already scheduled.
Signed-off-by Damien Le Moal <[email protected]>--- block/blk-zoned.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)block/blk-zoned.c +3
-3@@ -1205,13 +1205,15 @@ static void disk_zone_wplug_schedule_bio_work(structgendisk *disk, lockdep_assert_held(&zwplug->lock); /* -* Take a reference on the zone write plug and schedule the submission -* of the next plugged BIO. blk_zone_wplug_bio_work() will release the -* reference we take here. +* Schedule the submission of the next plugged BIO. If the zone write +* plug BIO work is not already scheduled, take a reference on the zone +* write plug to ensure that it does not go away while the work is being block/blk-zoned.c static void disk_zone_wplug_schedule_bio_work(struct · line 1218
Quoted 10 lines
block/blk-zoned.c +4
-2+* scheduled but has not run yet. blk_zone_wplug_bio_work() will release +* the reference we take here. */ WARN_ON_ONCE(!(zwplug->flags & BLK_ZONE_WPLUG_PLUGGED)); - refcount_inc(&zwplug->ref); - queue_work(disk->zone_wplugs_wq, &zwplug->bio_work); + if (queue_work(disk->zone_wplugs_wq, &zwplug->bio_work)) + refcount_inc(&zwplug->ref); } static inline void disk_zone_wplug_add_bio(struct gendisk *disk,Quoted 7 lines
Urgh. Don't.
There is a race condition; 'bio_work' might be scheduled directly and complete before 'refcount_inc()' is called.
You have to invert the statement by keeping the 'refcount_inc()' before calling 'queue_work()', and then drop the reference again ifqueue_workfailed.
Ugly, I know.
Quoted 5 lines
As Bart pointed out, since this is done with the zone write plug spinlock held and the work takes that lock first when it runs, there is no chance that the refcount gets bad. That is indeed not super clear though, so I will imrove the comment to describe that.
But then we deadlock.
If a lock is held when calling queue_work() and the workqueue function is calling spin_lock() on the _same_ lock _and_ we are scheduled on the _same_ CPU (remember single-core systems ?) we deadlock. Don't.
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Re: [PATCH 4/8] block: improve disk_zone_wplug_schedule_bio_work()
Quoted 4 lines · Hannes Reinecke
But then we deadlock.
If a lock is held when callingqueue_work()and the workqueue function is callingspin_lock()on the _same_ lock _and_ we are scheduled on the _same_ CPU (remember single-core systems ?) we deadlock.
You very obviously can't context switch to a separate thread when holding a spinlock.
Note that I particularly like this pattern. I'd much rather just increment the reference and drop it when we did not schedule the work, this is not a super hot path that needs crazy optimizations, and that would have avoided this bike shedding discussion.
Re: [PATCH 4/8] block: improve disk_zone_wplug_schedule_bio_work()
In reply to Damien Le MoalDamien Le Moal
maintainerPATCH 5/8 block: rename struct gendisk zone_wplugs_lock field
In reply to Damien Le MoalRename struct gendisk zone_wplugs_lock field to zone_wplugs_hash_lock to clearly indicates that this is the spinlock used for manipulating the hash table of zone write plugs.
--- block/blk-zoned.c | 23 ++++++++++++----------- include/linux/blkdev.h | 2 +- 2 files changed, 13 insertions(+), 12 deletions(-)
block/blk-zoned.c +12 -11
@@ -506,10 +506,11 @@ static bool disk_insert_zone_wplug(struct gendisk *disk,* are racing with other submission context, so we may already have a * zone write plug for the same zone. */ -spin_lock_irqsave (&disk->zone_wplugs_lock, flags); +spin_lock_irqsave (&disk->zone_wplugs_hash_lock, flags);hlist_for_each_entry_rcu (zwplg, &disk->zone_wplugs_hash[idx], node) {if (zwplg->zone_no == zwplug->zone_no) { -spin_unlock_irqrestore (&disk->zone_wplugs_lock, flags); +spin_unlock_irqrestore (&disk->zone_wplugs_hash_lock, + flags);return false; } }
@@ -521,7 +522,7 @@ static bool disk_insert_zone_wplug(struct gendisk *disk,* necessarilly in the active condition. */ zones_cond =rcu_dereference_check (disk->zones_cond, -lockdep_is_held (&disk->zone_wplugs_lock)); +lockdep_is_held (&disk->zone_wplugs_hash_lock));if (zones_cond) zwplug->cond = zones_cond[zwplug->zone_no];else
@@ -529,7 +530,7 @@ static bool disk_insert_zone_wplug(struct gendisk *disk,hlist_add_head_rcu (&zwplug->node, &disk->zone_wplugs_hash[idx]);atomic_inc (&disk->nr_zone_wplugs); -spin_unlock_irqrestore (&disk->zone_wplugs_lock, flags); +spin_unlock_irqrestore (&disk->zone_wplugs_hash_lock, flags);return true; }
@@ -629,13 +630,13 @@ static void disk_remove_zone_wplug(struct gendisk *disk,* the zone write plug and drop the extra reference we took when the * plug was inserted in the hash table. */ -spin_lock_irqsave (&disk->zone_wplugs_lock, flags); +spin_lock_irqsave (&disk->zone_wplugs_hash_lock, flags);blk_zone_set_cond (rcu_dereference_check (disk->zones_cond, -lockdep_is_held (&disk->zone_wplugs_lock)), +lockdep_is_held (&disk->zone_wplugs_hash_lock)), zwplug->zone_no, zwplug->cond);hlist_del_init_rcu (&zwplug->node);atomic_dec (&disk->nr_zone_wplugs); -spin_unlock_irqrestore (&disk->zone_wplugs_lock, flags); +spin_unlock_irqrestore (&disk->zone_wplugs_hash_lock, flags);disk_put_zone_wplug (zwplug); }
@@ -1784,7 +1785,7 @@ static void blk_zone_wplug_bio_work(struct work_struct *work)void disk_init_zone_resources (struct gendisk *disk) { -spin_lock_init (&disk->zone_wplugs_lock); +spin_lock_init (&disk->zone_wplugs_hash_lock); }/*
@@ -1874,10 +1875,10 @@ static void disk_set_zones_cond_array(struct gendisk *disk, u8 *zones_cond) {unsigned long flags; -spin_lock_irqsave (&disk->zone_wplugs_lock, flags); +spin_lock_irqsave (&disk->zone_wplugs_hash_lock, flags); zones_cond =rcu_replace_pointer (disk->zones_cond, zones_cond, -lockdep_is_held (&disk->zone_wplugs_lock)); -spin_unlock_irqrestore (&disk->zone_wplugs_lock, flags); +lockdep_is_held (&disk->zone_wplugs_hash_lock)); +spin_unlock_irqrestore (&disk->zone_wplugs_hash_lock, flags);kfree_rcu_mightsleep (zones_cond); }
include/linux/blkdev.h +1 -1
@@ -200,7 +200,7 @@ struct gendisk {u8 __rcu *zones_cond;unsigned int zone_wplugs_hash_bits;atomic_t nr_zone_wplugs; -spinlock_t zone_wplugs_lock; +spinlock_t zone_wplugs_hash_lock;struct mempool *zone_wplugs_pool;struct hlist_head *zone_wplugs_hash;struct workqueue_struct *zone_wplugs_wq;
Signature · 1 line
2.53.0
Re: [PATCH 5/8] block: rename struct gendisk zone_wplugs_lock field
Quoted 10 lines · Damien Le Moal
Rename struct gendisk
zone_wplugs_lockfield tozone_wplugs_hash_lockto clearly indicates that this is the spinlock used for manipulating the hash table of zone write plugs.Signed-off-by Damien Le Moal <[email protected]>--- block/blk-zoned.c | 23 ++++++++++++----------- include/linux/blkdev.h | 2 +- 2 files changed, 13 insertions(+), 12 deletions(-)
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Re: [PATCH 5/8] block: rename struct gendisk zone_wplugs_lock field
In reply to Damien Le MoalLooks good,
Damien Le Moal
maintainerPATCH 6/8 block: allow submitting all zone writes from a single context
In reply to Damien Le MoalIn order to maintain sequential write patterns per zone with zoned block devices, zone write plugging issues only a single write BIO per zone at any time. This works well but has the side effect that when large sequential write streams are issued by the user and these streams cross zone boundaries, the device ends up receiving a discontiguous set of write commands for different zones. The same also happens when a user writes simultaneously at high queue depth multiple zones: the device does not see all sequential writes per zone and receives discontiguous writes to different zones. While this does not affect the performance of solid state zoned block devices, when using an SMR HDD, this pattern change from sequential writes to discontiguous writes to different zones significantly increases head seek which results in degraded write throughput.
In order to reduce this seek overhead for rotational media devices, introduce a per disk zone write plugs kernel thread to issue all write BIOs to zones. This single zone write issuing context is enabled for any zoned block device that has a request queue flagged with the new QUEUE_ZONED_QD1_WRITES flag.
The flag QUEUE_ZONED_QD1_WRITES is visible as the sysfs queue attribute zoned_qd1_writes. For a regular block device, this attribute cannot be changed and is always 0. For zoned block devices, a user can override the default value set to force the global write maximum queue depth of 1 for a zoned block device, or clear this attribute to fallback to the default behavior of zone write plugging which limits writes to QD=1 per sequential zone.
Writing to a zoned block device flagged with QUEUE_ZONED_QD1_WRITES is implemented using a list of zone write plugs that have a non-empty BIO list. Listed zone write plugs are processed by the disk zone write plugs worker kthread in FIFO order, and all BIOs of a zone write plug are all processed before switching to the next listed zone write plug. A newly submitted BIO for a non-FULL zone write plug that is not yet listed causes the addition of the zone write plug at the end of the disk list of zone write plugs.
Since the write BIOs queued in a zone write plug BIO list are necessarilly sequential, for rotational media, using the single zone write plugs kthread to issue all BIOs maintains a sequential write pattern and thus reduces seek overhead and improves write throughput. This processing essentially result in always writing to HDDs at QD=1, which is not an issue for HDDs operating with write caching enabled. Performance with write cache disabled is also not degraded thanks to the efficient write handling of modern SMR HDDs.
A disk list of zone write plugs is defined using the new struct gendisk zone_wplugs_list, and accesses to this list is protected using the zone_wplugs_list_lock spinlock. The per disk kthread (zone_wplugs_worker) code is implemented by the function disk_zone_wplugs_worker(). A reference on listed zone write plugs is always held until all BIOs of the zone write plug are processed by the worker kthread. BIO issuing at QD=1 is driven using a completion structure (zone_wplugs_worker_bio_done) and calls to blk_io_wait().
With this change, performance when sequentially writing the zones of a 30 TB SMR SATA HDD connected to an AHCI adapter changes as follows (1MiB direct I/Os, results in MB/s unit):
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Sequential write | Baseline | Patched | | Queue Depth | 6.19-rc8 | | +------------------+----------+---------+ | 1 | 244 | 245 | | 2 | 244 | 245 | | 4 | 245 | 245 | | 8 | 242 | 245 | | 16 | 222 | 246 | | 32 | 211 | 245 | | 64 | 193 | 244 | | 128 | 112 | 246 | +------------------+----------+---------+
With the current code (baseline), as the sequential write stream crosses a zone boundary, higher queue depth creates a gap between the last IO to the previous zone and the first IOs to the following zones, causing head seeks and degrading performance. Using the disk zone write plugs worker thread, this pattern disappears and the maximum throughput of the drive is maintained, leading to over 100% improvements in throughput for high queue depth write.
Using 16 fio jobs all writing to randomly chosen zones at QD=32 with 1 MiB direct IOs, write throughput also increases significantly.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Random write | Baseline | Patched | | Number of zones | 6.19-rc7 | | +------------------+----------+---------+ | 1 | 191 | 192 | | 2 | 101 | 128 | | 4 | 115 | 123 | | 8 | 90 | 120 | | 16 | 64 | 115 | | 32 | 58 | 105 | | 64 | 56 | 101 | | 128 | 55 | 99 | +------------------+----------+---------+
Tests using XFS shows that buffered write speed with 8 jobs writing files increases by 12% to 35% depending on the workload.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Workload | Baseline | Patched | | | 6.19-rc7 | | +------------------+----------+---------+ | 256MiB file size | 212 | 238 | +------------------+----------+---------+ | 4MiB .. 128 MiB | 213 | 243 | | random file size | | | +------------------+----------+---------+ | 2MiB .. 8 MiB | 179 | 242 | | random file size | | | +------------------+----------+---------+
Performance gains are even more significant when using an HBA that limits the maximum size of commands to a small value, e.g. HBAs controlled with the mpi3mr driver limit commands to a maximum of 1 MiB. In such case, the write throughput gains are over 40%.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Workload | Baseline | Patched | | | 6.19-rc7 | | +------------------+----------+---------+ | 256MiB file size | 175 | 245 | +------------------+----------+---------+ | 4MiB .. 128 MiB | 174 | 244 | | random file size | | | +------------------+----------+---------+ | 2MiB .. 8 MiB | 171 | 243 | | random file size | | | +------------------+----------+---------+
--- block/blk-mq-debugfs.c | 1 + block/blk-sysfs.c | 35 ++++++++ block/blk-zoned.c | 197 +++++++++++++++++++++++++++++++++++------ include/linux/blkdev.h | 8 ++ 4 files changed, 215 insertions(+), 26 deletions(-)
block/blk-mq-debugfs.c +1 -0
@@ -97,6 +97,7 @@ static const char *const blk_queue_flag_name[] = {QUEUE_FLAG_NAME (NO_ELV_SWITCH ),QUEUE_FLAG_NAME (QOS_ENABLED ),QUEUE_FLAG_NAME (BIO_ISSUE_TIME ), + QUEUE_FLAG_NAME(ZONED_QD1_WRITES), };#undef QUEUE_FLAG_NAME
block/blk-sysfs.c +35 -0
@@ -384,6 +384,39 @@ static ssize_t queue_nr_zones_show(struct gendisk *disk, char *page)return queue_var_show (disk_nr_zones (disk), page); } +static ssize_t queue_zoned_qd1_writes_show (struct gendisk *disk,char *page) +{ +return queue_var_show (!!blk_queue_zoned_qd1_writes (disk->queue), + page); +} + +static ssize_t queue_zoned_qd1_writes_store (struct gendisk *disk, +const char *page,size_t count) +{ +struct request_queue *q = disk->queue; +unsigned long qd1_writes; +unsigned int memflags; +ssize_t ret; + +if (!blk_queue_is_zoned (q)) +return -EOPNOTSUPP ; + + ret =queue_var_store (&qd1_writes, page, count); +if (ret <0 ) +return ret; + + memflags =blk_mq_freeze_queue (q); +blk_mq_quiesce_queue (q); +if (qd1_writes) +blk_queue_flag_set (QUEUE_FLAG_ZONED_QD1_WRITES , q); +else +blk_queue_flag_clear (QUEUE_FLAG_ZONED_QD1_WRITES , q); +blk_mq_unquiesce_queue (q); +blk_mq_unfreeze_queue (q, memflags); + +return count; +} +static ssize_t queue_iostats_passthrough_show (struct gendisk *disk,char *page) {return queue_var_show (!!blk_queue_passthrough_stat (disk->queue), page);
@@ -611,6 +644,7 @@ QUEUE_LIM_RO_ENTRY(queue_max_zone_append_sectors, "zone_append_max_bytes");QUEUE_LIM_RO_ENTRY (queue_zone_write_granularity,"zone_write_granularity" );QUEUE_LIM_RO_ENTRY (queue_zoned,"zoned" ); +QUEUE_RW_ENTRY (queue_zoned_qd1_writes,"zoned_qd1_writes" );QUEUE_RO_ENTRY (queue_nr_zones,"nr_zones" );QUEUE_LIM_RO_ENTRY (queue_max_open_zones,"max_open_zones" );QUEUE_LIM_RO_ENTRY (queue_max_active_zones,"max_active_zones" );
@@ -748,6 +782,7 @@ static struct attribute *queue_attrs[] = { &queue_nomerges_entry.attr, &queue_poll_entry.attr, &queue_poll_delay_entry.attr, + &queue_zoned_qd1_writes_entry.attr, NULL, };
block/blk-zoned.c +171 -26
338 lines not on this page · raw message
include/linux/blkdev.h +8 -0
@@ -13,6 +13,7 @@#include <linux/minmax.h> #include <linux/timer.h> #include <linux/workqueue.h> +#include <linux/completion.h> #include <linux/wait.h> #include <linux/bio.h> #include <linux/gfp.h>
@@ -204,6 +205,10 @@ struct gendisk {struct mempool *zone_wplugs_pool;struct hlist_head *zone_wplugs_hash;struct workqueue_struct *zone_wplugs_wq; +spinlock_t zone_wplugs_list_lock; +struct list_head zone_wplugs_list; +struct task_struct *zone_wplugs_worker; +struct completion zone_wplugs_worker_bio_done;#endif /* CONFIG_BLK_DEV_ZONED */ #if IS_ENABLED (CONFIG_CDROM )
@@ -668,6 +673,7 @@ enum {QUEUE_FLAG_NO_ELV_SWITCH ,/* can't switch elevator any more */ QUEUE_FLAG_QOS_ENABLED ,/* qos is enabled */ QUEUE_FLAG_BIO_ISSUE_TIME ,/* record bio->issue_time_ns */ +QUEUE_FLAG_ZONED_QD1_WRITES ,/* Limit zoned devices writes to QD=1 */ QUEUE_FLAG_MAX };
@@ -707,6 +713,8 @@ void blk_queue_flag_clear(unsigned int flag, struct request_queue *q); test_bit(QUEUE_FLAG_DISABLE_WBT_DEF, &(q)->queue_flags)#define blk_queue_no_elv_switch (q) \ test_bit(QUEUE_FLAG_NO_ELV_SWITCH, &(q)->queue_flags) +#define blk_queue_zoned_qd1_writes (q) \ + test_bit(QUEUE_FLAG_ZONED_QD1_WRITES, &(q)->queue_flags)extern void blk_set_pm_only (struct request_queue *q);extern void blk_clear_pm_only (struct request_queue *q);
Signature · 1 line
2.53.0
Re: [PATCH 6/8] block: allow submitting all zone writes from a single context
Quoted 213 lines · Damien Le Moal
In order to maintain sequential write patterns per zone with zoned block devices, zone write plugging issues only a single write BIO per zone at any time. This works well but has the side effect that when large sequential write streams are issued by the user and these streams cross zone boundaries, the device ends up receiving a discontiguous set of write commands for different zones. The same also happens when a user writes simultaneously at high queue depth multiple zones: the device does not see all sequential writes per zone and receives discontiguous writes to different zones. While this does not affect the performance of solid state zoned block devices, when using an SMR HDD, this pattern change from sequential writes to discontiguous writes to different zones significantly increases head seek which results in degraded write throughput.
In order to reduce this seek overhead for rotational media devices, introduce a per disk zone write plugs kernel thread to issue all write BIOs to zones. This single zone write issuing context is enabled for any zoned block device that has a request queue flagged with the new
QUEUE_ZONED_QD1_WRITESflag.The flag
QUEUE_ZONED_QD1_WRITESis visible as the sysfs queue attributezoned_qd1_writes. For a regular block device, this attribute cannot be changed and is always 0. For zoned block devices, a user can override the default value set to force the global write maximum queue depth of 1 for a zoned block device, or clear this attribute to fallback to the default behavior of zone write plugging which limits writes to QD=1 per sequential zone.Writing to a zoned block device flagged with
QUEUE_ZONED_QD1_WRITESis implemented using a list of zone write plugs that have a non-empty BIO list. Listed zone write plugs are processed by the disk zone write plugs worker kthread in FIFO order, and all BIOs of a zone write plug are all processed before switching to the next listed zone write plug. A newly submitted BIO for a non-FULL zone write plug that is not yet listed causes the addition of the zone write plug at the end of the disk list of zone write plugs.Since the write BIOs queued in a zone write plug BIO list are necessarilly sequential, for rotational media, using the single zone write plugs kthread to issue all BIOs maintains a sequential write pattern and thus reduces seek overhead and improves write throughput. This processing essentially result in always writing to HDDs at QD=1, which is not an issue for HDDs operating with write caching enabled. Performance with write cache disabled is also not degraded thanks to the efficient write handling of modern SMR HDDs.
A disk list of zone write plugs is defined using the new struct gendisk
zone_wplugs_list, and accesses to this list is protected using thezone_wplugs_list_lockspinlock. The per disk kthread (zone_wplugs_worker) code is implemented by the functiondisk_zone_wplugs_worker(). A reference on listed zone write plugs is always held until all BIOs of the zone write plug are processed by the worker kthread. BIO issuing at QD=1 is driven using a completion structure (zone_wplugs_worker_bio_done) and calls toblk_io_wait().With this change, performance when sequentially writing the zones of a 30 TB SMR SATA HDD connected to an AHCI adapter changes as follows (1MiB direct I/Os, results in MB/s unit):
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Sequential write | Baseline | Patched | | Queue Depth | 6.19-rc8 | | +------------------+----------+---------+ | 1 | 244 | 245 | | 2 | 244 | 245 | | 4 | 245 | 245 | | 8 | 242 | 245 | | 16 | 222 | 246 | | 32 | 211 | 245 | | 64 | 193 | 244 | | 128 | 112 | 246 | +------------------+----------+---------+With the current code (baseline), as the sequential write stream crosses a zone boundary, higher queue depth creates a gap between the last IO to the previous zone and the first IOs to the following zones, causing head seeks and degrading performance. Using the disk zone write plugs worker thread, this pattern disappears and the maximum throughput of the drive is maintained, leading to over 100% improvements in throughput for high queue depth write.
Using 16 fio jobs all writing to randomly chosen zones at QD=32 with 1 MiB direct IOs, write throughput also increases significantly.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Random write | Baseline | Patched | | Number of zones | 6.19-rc7 | | +------------------+----------+---------+ | 1 | 191 | 192 | | 2 | 101 | 128 | | 4 | 115 | 123 | | 8 | 90 | 120 | | 16 | 64 | 115 | | 32 | 58 | 105 | | 64 | 56 | 101 | | 128 | 55 | 99 | +------------------+----------+---------+Tests using XFS shows that buffered write speed with 8 jobs writing files increases by 12% to 35% depending on the workload.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Workload | Baseline | Patched | | | 6.19-rc7 | | +------------------+----------+---------+ | 256MiB file size | 212 | 238 | +------------------+----------+---------+ | 4MiB .. 128 MiB | 213 | 243 | | random file size | | | +------------------+----------+---------+ | 2MiB .. 8 MiB | 179 | 242 | | random file size | | | +------------------+----------+---------+Performance gains are even more significant when using an HBA that limits the maximum size of commands to a small value, e.g. HBAs controlled with the mpi3mr driver limit commands to a maximum of 1 MiB. In such case, the write throughput gains are over 40%.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Workload | Baseline | Patched | | | 6.19-rc7 | | +------------------+----------+---------+ | 256MiB file size | 175 | 245 | +------------------+----------+---------+ | 4MiB .. 128 MiB | 174 | 244 | | random file size | | | +------------------+----------+---------+ | 2MiB .. 8 MiB | 171 | 243 | | random file size | | | +------------------+----------+---------+Signed-off-by Damien Le Moal <[email protected]>--- block/blk-mq-debugfs.c | 1 + block/blk-sysfs.c | 35 ++++++++ block/blk-zoned.c | 197 +++++++++++++++++++++++++++++++++++------ include/linux/blkdev.h | 8 ++ 4 files changed, 215 insertions(+), 26 deletions(-)block/blk-mq-debugfs.c +1
-0@@ -97,6 +97,7 @@ static const char *const blk_queue_flag_name[] = {QUEUE_FLAG_NAME (NO_ELV_SWITCH ),QUEUE_FLAG_NAME (QOS_ENABLED ),QUEUE_FLAG_NAME (BIO_ISSUE_TIME ), + QUEUE_FLAG_NAME(ZONED_QD1_WRITES), };#undef QUEUE_FLAG_NAMEblock/blk-sysfs.c +34
-0@@ -384,6 +384,39 @@ static ssize_t queue_nr_zones_show(struct gendisk *disk, char *page)return queue_var_show (disk_nr_zones (disk), page); } +static ssize_t queue_zoned_qd1_writes_show (struct gendisk *disk,char *page) +{ +return queue_var_show (!!blk_queue_zoned_qd1_writes (disk->queue), + page); +} + +static ssize_t queue_zoned_qd1_writes_store (struct gendisk *disk, +const char *page,size_t count) +{ +struct request_queue *q = disk->queue; +unsigned long qd1_writes; +unsigned int memflags; +ssize_t ret; + +if (!blk_queue_is_zoned (q)) +return -EOPNOTSUPP ; + + ret =queue_var_store (&qd1_writes, page, count); +if (ret <0 ) +return ret; + + memflags =blk_mq_freeze_queue (q); +blk_mq_quiesce_queue (q); +if (qd1_writes) +blk_queue_flag_set (QUEUE_FLAG_ZONED_QD1_WRITES , q); +else +blk_queue_flag_clear (QUEUE_FLAG_ZONED_QD1_WRITES , q); +blk_mq_unquiesce_queue (q); +blk_mq_unfreeze_queue (q, memflags); + +return count; +} +static ssize_t queue_iostats_passthrough_show (struct gendisk *disk,char *page) {return queue_var_show (!!blk_queue_passthrough_stat (disk->queue), page);@@ -611,6 +644,7 @@ QUEUE_LIM_RO_ENTRY(queue_max_zone_append_sectors, "zone_append_max_bytes");QUEUE_LIM_RO_ENTRY (queue_zone_write_granularity,"zone_write_granularity" );QUEUE_LIM_RO_ENTRY (queue_zoned,"zoned" ); +QUEUE_RW_ENTRY (queue_zoned_qd1_writes,"zoned_qd1_writes" );QUEUE_RO_ENTRY (queue_nr_zones,"nr_zones" );QUEUE_LIM_RO_ENTRY (queue_max_open_zones,"max_open_zones" );QUEUE_LIM_RO_ENTRY (queue_max_active_zones,"max_active_zones" );
block/blk-sysfs.c static struct attribute *queue_attrs[] = { · line 788
Quoted 7 lines
block/blk-sysfs.c +1
-0@@ -748,6 +782,7 @@ static struct attribute *queue_attrs[] = { &queue_nomerges_entry.attr, &queue_poll_entry.attr, &queue_poll_delay_entry.attr, + &queue_zoned_qd1_writes_entry.attr, NULL, };
Can't you plug it into the 'queue_attr_visible()' function, too, such that it doesn't even show up for non-zoned drives? (After all, it's not that you can change between zoned and non-zoned drives.)
(One hopes :-)
And I really had hoped that we wouldn't need to introduce new kthreads, rather that workqueue are the new kthreads. Any reasoning why you couldn't use workqueues here?
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Damien Le Moal
maintainerRe: [PATCH 6/8] block: allow submitting all zone writes from a single context
Quoted 218 lines · Hannes Reinecke
Quoted 217 lines · Damien Le Moal
In order to maintain sequential write patterns per zone with zoned block devices, zone write plugging issues only a single write BIO per zone at any time. This works well but has the side effect that when large sequential write streams are issued by the user and these streams cross zone boundaries, the device ends up receiving a discontiguous set of write commands for different zones. The same also happens when a user writes simultaneously at high queue depth multiple zones: the device does not see all sequential writes per zone and receives discontiguous writes to different zones. While this does not affect the performance of solid state zoned block devices, when using an SMR HDD, this pattern change from sequential writes to discontiguous writes to different zones significantly increases head seek which results in degraded write throughput.
In order to reduce this seek overhead for rotational media devices, introduce a per disk zone write plugs kernel thread to issue all write BIOs to zones. This single zone write issuing context is enabled for any zoned block device that has a request queue flagged with the new
QUEUE_ZONED_QD1_WRITESflag.The flag
QUEUE_ZONED_QD1_WRITESis visible as the sysfs queue attributezoned_qd1_writes. For a regular block device, this attribute cannot be changed and is always 0. For zoned block devices, a user can override the default value set to force the global write maximum queue depth of 1 for a zoned block device, or clear this attribute to fallback to the default behavior of zone write plugging which limits writes to QD=1 per sequential zone.Writing to a zoned block device flagged with
QUEUE_ZONED_QD1_WRITESis implemented using a list of zone write plugs that have a non-empty BIO list. Listed zone write plugs are processed by the disk zone write plugs worker kthread in FIFO order, and all BIOs of a zone write plug are all processed before switching to the next listed zone write plug. A newly submitted BIO for a non-FULL zone write plug that is not yet listed causes the addition of the zone write plug at the end of the disk list of zone write plugs.Since the write BIOs queued in a zone write plug BIO list are necessarilly sequential, for rotational media, using the single zone write plugs kthread to issue all BIOs maintains a sequential write pattern and thus reduces seek overhead and improves write throughput. This processing essentially result in always writing to HDDs at QD=1, which is not an issue for HDDs operating with write caching enabled. Performance with write cache disabled is also not degraded thanks to the efficient write handling of modern SMR HDDs.
A disk list of zone write plugs is defined using the new struct gendisk
zone_wplugs_list, and accesses to this list is protected using thezone_wplugs_list_lockspinlock. The per disk kthread (zone_wplugs_worker) code is implemented by the functiondisk_zone_wplugs_worker(). A reference on listed zone write plugs is always held until all BIOs of the zone write plug are processed by the worker kthread. BIO issuing at QD=1 is driven using a completion structure (zone_wplugs_worker_bio_done) and calls toblk_io_wait().With this change, performance when sequentially writing the zones of a 30 TB SMR SATA HDD connected to an AHCI adapter changes as follows (1MiB direct I/Os, results in MB/s unit):
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Sequential write | Baseline | Patched | | Queue Depth | 6.19-rc8 | | +------------------+----------+---------+ | 1 | 244 | 245 | | 2 | 244 | 245 | | 4 | 245 | 245 | | 8 | 242 | 245 | | 16 | 222 | 246 | | 32 | 211 | 245 | | 64 | 193 | 244 | | 128 | 112 | 246 | +------------------+----------+---------+With the current code (baseline), as the sequential write stream crosses a zone boundary, higher queue depth creates a gap between the last IO to the previous zone and the first IOs to the following zones, causing head seeks and degrading performance. Using the disk zone write plugs worker thread, this pattern disappears and the maximum throughput of the drive is maintained, leading to over 100% improvements in throughput for high queue depth write.
Using 16 fio jobs all writing to randomly chosen zones at QD=32 with 1 MiB direct IOs, write throughput also increases significantly.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Random write | Baseline | Patched | | Number of zones | 6.19-rc7 | | +------------------+----------+---------+ | 1 | 191 | 192 | | 2 | 101 | 128 | | 4 | 115 | 123 | | 8 | 90 | 120 | | 16 | 64 | 115 | | 32 | 58 | 105 | | 64 | 56 | 101 | | 128 | 55 | 99 | +------------------+----------+---------+Tests using XFS shows that buffered write speed with 8 jobs writing files increases by 12% to 35% depending on the workload.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Workload | Baseline | Patched | | | 6.19-rc7 | | +------------------+----------+---------+ | 256MiB file size | 212 | 238 | +------------------+----------+---------+ | 4MiB .. 128 MiB | 213 | 243 | | random file size | | | +------------------+----------+---------+ | 2MiB .. 8 MiB | 179 | 242 | | random file size | | | +------------------+----------+---------+Performance gains are even more significant when using an HBA that limits the maximum size of commands to a small value, e.g. HBAs controlled with the mpi3mr driver limit commands to a maximum of 1 MiB. In such case, the write throughput gains are over 40%.
+--------------------+ | Write BW (MB/s) | +------------------+----------+---------+ | Workload | Baseline | Patched | | | 6.19-rc7 | | +------------------+----------+---------+ | 256MiB file size | 175 | 245 | +------------------+----------+---------+ | 4MiB .. 128 MiB | 174 | 244 | | random file size | | | +------------------+----------+---------+ | 2MiB .. 8 MiB | 171 | 243 | | random file size | | | +------------------+----------+---------+Signed-off-by Damien Le Moal <[email protected]>---
block/blk-mq-debugfs.c| 1 +
block/blk-sysfs.c | 35 ++++++++
block/blk-zoned.c | 197 +++++++++++++++++++++++++++++++++++------include/linux/blkdev.h| 8 ++4 files changed, 215 insertions(+), 26 deletions(-)block/blk-mq-debugfs.c +1
-0@@ -97,6 +97,7 @@ static const char *const blk_queue_flag_name[] = { QUEUE_FLAG_NAME(NO_ELV_SWITCH), QUEUE_FLAG_NAME(QOS_ENABLED), QUEUE_FLAG_NAME(BIO_ISSUE_TIME), + QUEUE_FLAG_NAME(ZONED_QD1_WRITES), }; #undef QUEUE_FLAG_NAMEdiff --git
a/block/blk-sysfs.cb/block/blk-sysfs.cindexf3b1968c80ce..789802286d95100644block/blk-sysfs.c +32
-0@@ -384,6 +384,39 @@ static ssize_t queue_nr_zones_show(struct gendisk *disk, char *page) return queue_var_show(disk_nr_zones(disk), page); } +static ssize_t queue_zoned_qd1_writes_show(struct gendisk *disk, char *page) +{ + return queue_var_show(!!blk_queue_zoned_qd1_writes(disk->queue), + page); +} + +static ssize_t queue_zoned_qd1_writes_store (struct gendisk *disk, + const char *page, size_t count) +{ + struct request_queue *q = disk->queue; + unsigned long qd1_writes; + unsigned int memflags; + ssize_t ret; + + if (!blk_queue_is_zoned(q)) + return -EOPNOTSUPP; + + ret = queue_var_store(&qd1_writes, page, count); + if (ret < 0) + return ret; + + memflags = blk_mq_freeze_queue(q); + blk_mq_quiesce_queue(q); + if (qd1_writes) + blk_queue_flag_set(QUEUE_FLAG_ZONED_QD1_WRITES, q); + else + blk_queue_flag_clear(QUEUE_FLAG_ZONED_QD1_WRITES, q); + blk_mq_unquiesce_queue(q); + blk_mq_unfreeze_queue(q, memflags); + + return count; +} +static
ssize_tqueue_iostats_passthrough_show(struct gendisk *disk, char *page)
{
returnqueue_var_show(!!blk_queue_passthrough_stat(disk->queue), page);block/blk-sysfs.c +2
-0@@ -611,6 +644,7 @@ QUEUE_LIM_RO_ENTRY(queue_max_zone_append_sectors, "zone_append_max_bytes"); QUEUE_LIM_RO_ENTRY(queue_zone_write_granularity, "zone_write_granularity"); QUEUE_LIM_RO_ENTRY(queue_zoned, "zoned"); +QUEUE_RW_ENTRY (queue_zoned_qd1_writes,"zoned_qd1_writes" ); QUEUE_RO_ENTRY(queue_nr_zones, "nr_zones"); QUEUE_LIM_RO_ENTRY(queue_max_open_zones, "max_open_zones"); QUEUE_LIM_RO_ENTRY(queue_max_active_zones, "max_active_zones");@@ -748,6 +782,7 @@ static struct attribute *queue_attrs[] = { &queue_nomerges_entry.attr, &queue_poll_entry.attr, &queue_poll_delay_entry.attr, + &queue_zoned_qd1_writes_entry.attr,
Quoted 8 lines
block/blk-sysfs.c static struct attribute *queue_attrs[] = { · line 787
Quoted 2 lines
block/blk-sysfs.c +0
-0NULL, };Can't you plug it into the '
queue_attr_visible()' function, too, such that it doesn't even show up for non-zoned drives? (After all, it's not that you can change between zoned and non-zoned drives.)
(One hopes :-)
Good point. Will look into that.
Quoted 3 lines
And I really had hoped that we wouldn't need to introduce new kthreads, rather that workqueue are the new kthreads. Any reasoning why you couldn't use workqueues here?
I can use a single work item instead of a kthread. I had that initially, but the code ended up cleaner with the kthread. I can try again with the work item if you insist...
Signature · 2 lines
Damien Le Moal Western Digital Research
Damien Le Moal
maintainerPATCH 7/8 block: default to QD=1 writes for blk-mq rotational zoned devices
In reply to Damien Le MoalFor blk-mq rotational zoned block devices (e.g. SMR HDDs), default to having zone write plugging limit write operations to a maximum queue depth of 1 for all zones. This significantly reduce write seek overhead and improves SMR HDD write throughput.
For remotely connected disks with a very high network latency this features might not be useful. However, remotely connected zoned devices are rare at the moment, and we cannot know the round trip latency to pick a good default for network attached devices. System administrators can however disable this feature in that case.
For BIO based (non blk-mq) rotational zoned block devices, the device driver (e.g. a DM target driver) can directly set an appropriate default.
--- block/blk-sysfs.c | 8 ++++++++ 1 file changed, 8 insertions(+)
block/blk-sysfs.c +8 -0
@@ -963,6 +963,14 @@ int blk_register_queue(struct gendisk *disk)blk_mq_debugfs_register (q);blk_debugfs_unlock (q, memflags); +/* +* For blk-mq rotational zoned devices, default to using QD=1 +* writes. For non-mq rotational zoned devices, the device driver can +* set an appropriate default. +*/ +if (queue_is_mq (q) &&blk_queue_rot (q) &&blk_queue_is_zoned (q)) +blk_queue_flag_set (QUEUE_FLAG_ZONED_QD1_WRITES , q); + ret =disk_register_independent_access_ranges (disk);if (ret) goto out_debugfs_remove;
Signature · 1 line
2.53.0
Re: [PATCH 7/8] block: default to QD=1 writes for blk-mq rotational zoned devices
Quoted 11 lines · Damien Le Moal
For blk-mq rotational zoned block devices (e.g. SMR HDDs), default to having zone write plugging limit write operations to a maximum queue depth of 1 for all zones. This significantly reduce write seek overhead and improves SMR HDD write throughput.
For remotely connected disks with a very high network latency this features might not be useful. However, remotely connected zoned devices are rare at the moment, and we cannot know the round trip latency to pick a good default for network attached devices. System administrators can however disable this feature in that case.
Quoted 9 lines
For BIO based (non blk-mq) rotational zoned block devices, the device driver (e.g. a DM target driver) can directly set an appropriate default.
Signed-off-by Damien Le Moal <[email protected]>--- block/blk-sysfs.c | 8 ++++++++ 1 file changed, 8 insertions(+)
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Damien Le Moal
maintainerPATCH 8/8 Documentation: ABI: stable: document the zoned_qd1_writes attribute
In reply to Damien Le MoalUpdate the documentation file Documentation/ABI/stable/sysfs-block to describe the zoned_qd1_writes sysfs queue attribute file.
--- Documentation/ABI/stable/sysfs-block | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Documentation/ABI/stable/sysfs-block +15 -0
@@ -886,6 +886,21 @@ Description: zone commands, they will be treated as regular block devices and zoned will report "none". +What: /sys/block/<disk>/queue/zoned_qd1_writes +Date: January 2026 +Contact: Damien Le Moal <[email protected]> +Description: + [RW] zoned_qd1_writes indicates if write operations to a zoned + block device are being handled using a single issuer context (a + kernel thread) operating at a maximum queue depth of 1. A value + of 0 is always reported for regular block devices and for zoned + block devices that are not rotational devices (e.g. ZNS SSDs or + zoned UFS devices). For rotational zoned block devices (e.g. SMR + HDDs) the default value is 1. Since this default may not be + appropriate for some devices, e.g. remotely connected devices + over high latency networks, the user can disable this feature by + setting this attribute to 0. + What: /sys/block/<disk>/hidden Date: March 2023
Signature · 1 line
2.53.0
Re: [PATCH 8/8] Documentation: ABI: stable: document the zoned_qd1_writes attribute
Quoted 8 lines · Damien Le Moal
Update the documentation file
Documentation/ABI/stable/sysfs-blockto describe thezoned_qd1_writessysfs queue attribute file.Signed-off-by Damien Le Moal <[email protected]>--- Documentation/ABI/stable/sysfs-block | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
Cheers,
Hannes
Signature · 4 lines
Dr. Hannes Reinecke Kernel Storage Architect [email protected] +49 911 74053 688 SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich
Quoted 6 lines · Damien Le Moal
This patch series cleans up the zone write plugging code and introduces the ability to issue all write BIOs from a single context (a kthread) instead of allowing multiple zones to be written at the same time using a per zone work. As shown in patch 6, raw block device tests and XFS tests with an SMR HDD show that this can significantly increase write throughput (up to 40% over the current zone write plugging).
Hi Damien,
Is a new kthread necessary? Has the following approach been considered?
* Make the dm drivers that support rotational zoned storage devices request-based instead of bio-based.
* Modify blk_mq_get_tag() such that only one tag can be allocated at a time for zoned write requests.
I think that would be sufficient to serialize zoned writes. Additionally, this approach doesn't increase request processing latency by forcing a context switch to a kthread.
For reference, from drivers/md/dm.h:
/*
* To check whether the target type is request-based or not (bio-based). */
#define dm_target_request_based(t) ((t)->type->clone_and_map_rq != NULL)
Thanks,
Bart.
Damien Le Moal
maintainerQuoted 7 lines · Bart Van Assche
Quoted 6 lines · Damien Le Moal
This patch series cleans up the zone write plugging code and introduces the ability to issue all write BIOs from a single context (a kthread) instead of allowing multiple zones to be written at the same time using a per zone work. As shown in patch 6, raw block device tests and XFS tests with an SMR HDD show that this can significantly increase write throughput (up to 40% over the current zone write plugging).
Quoted 5 lines
Hi Damien,
Is a new kthread necessary? Has the following approach been considered?
* Make the dm drivers that support rotational zoned storage devices request-based instead of bio-based.
What you are suggesting would be an enormous amount of work (dm-linear, dm-flakey, dm-error, dm-crypt) to change generic code into DM targets that would be very specialized for just SMR HDDs. I do not understand why you think that would be a good idea.
Quoted 2 lines
* Modify
blk_mq_get_tag()such that only one tag can be allocated at a time for zoned write requests.
Sure, doing that would limit the number of write requests to zones to 1 at most at any time, but that would also result in a total loss of control over which zone write BIO work gets that single tag, meaning that the writes would in the end be mostly random again, like they are now. So with this solution, I can say goodbye to the +40% write throughput increase that I am seeing with the kthread.
Also note that this idea of limiting write tags combined with your idea of using req based DM targets would likely negatively impact dm-crypt performance as we would lose the ability to encrypt multiple writes in parallel on different CPUs.
Quoted 3 lines
I think that would be sufficient to serialize zoned writes. Additionally, this approach doesn't increase request processing latency
by forcing a context switch to a kthread.
This point is in my opinion moot because we currently use work items to issue the write commands. We are scheduling the zone write plugs BIO works in the submission and completion path, so the context switch overhead is already there. And would argue that using work items is potentially even more overhead than using a fixed kthread since the work items need to be assigned to CPUs and worker threads.
Granted, your point is valid for a QD=1 workload. In this case, I am indeed introducing a context switch where there is none now. But that is not really the use case we are looking at here. File system writeback does not happen at QD=1 per zone. Also, this added overhead does not really matter for HDDs anyway, and if that is really an issue, the user can enable the legacy zone write plugging behavior with "echo 0 > /sys/block/sdX/queue/zoned_qd1_writes".
Signature · 2 lines
Damien Le Moal Western Digital Research