On 3/24/24 21:44, Damien Le Moal wrote:
+/*
+ * Per-zone write plug.
+ */
+struct blk_zone_wplug {
+ struct hlist_node node;
+ struct list_head err;
+ atomic_t ref;
+ spinlock_t lock;
+ unsigned int flags;
+ unsigned int zone_no;
+ unsigned int wp_offset;
+ struct bio_list bio_list;
+ struct work_struct bio_work;
+};
Please document what 'lock' protects. Please also document the unit of
wp_offset.
Since there is an atomic reference count in this data structure, why is
the flag BLK_ZONE_WPLUG_FREEING required? Can that flag be replaced by
checking whether or not 'ref' is zero?
-void disk_free_zone_bitmaps(struct gendisk *disk)
+static bool disk_insert_zone_wplug(struct gendisk *disk,
+ struct blk_zone_wplug *zwplug)
+{
+ struct blk_zone_wplug *zwplg;
+ unsigned long flags;
+ unsigned int idx =
+ hash_32(zwplug->zone_no, disk->zone_wplugs_hash_bits);
+
+ /*
+ * Add the new zone write plug to the hash table, but carefully as we
+ * 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);
+ 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);
+ return false;
+ }
+ }
+ hlist_add_head_rcu(&zwplug->node, &disk->zone_wplugs_hash[idx]);
+ spin_unlock_irqrestore(&disk->zone_wplugs_lock, flags);
+
+ return true;
+}
Since this function inserts an element into disk->zone_wplugs_hash[],
can it happen that another thread removes that element from the hash
list before this function returns?
+static inline void disk_put_zone_wplug(struct blk_zone_wplug *zwplug)
+{
+ if (atomic_dec_and_test(&zwplug->ref)) {
+ WARN_ON_ONCE(!bio_list_empty(&zwplug->bio_list));
+ WARN_ON_ONCE(!list_empty(&zwplug->err));
+
+ kmem_cache_free(blk_zone_wplugs_cachep, zwplug);
+ }
+}
Calling kfree() or any of its variants for elements on an RCU-list
usually is not safe. If this is really safe in this case, a comment
should explain why.
+static struct blk_zone_wplug *disk_get_zone_wplug_locked(struct gendisk *disk,
+ sector_t sector, gfp_t gfp_mask,
+ unsigned long *flags)
What does the "_locked" suffix in the function name mean? Please add a
comment that explains this.
+static void disk_zone_wplug_abort_unaligned(struct gendisk *disk,
+ struct blk_zone_wplug *zwplug)
What does the "_unaligned" suffix in this function name mean? Please add
a comment that explains this.
Thanks,
Bart.
