Re: [PATCH v2 07/28] block: Introduce zone write plugging

Bart Van Assche
2024-03-25T21:53:26Z

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.