Re: [PATCH 03/15] block: add an API to atomically update queue limits

John Garry
2024-01-25T10:29:18Z

> +
> +int blk_validate_limits(struct queue_limits *lim)
> +{
> + unsigned int max_hw_sectors;
> +
> + if (!lim->logical_block_size)
> + lim->logical_block_size = SECTOR_SIZE;

nit: This function is doing a bit more than validating, as the function
name suggests

> +
> + if (!lim->physical_block_size)
> + lim->physical_block_size = SECTOR_SIZE;
> + if (lim->physical_block_size < lim->logical_block_size)
> + lim->physical_block_size = lim->physical_block_size;
> +
> + if (!lim->io_min)
> + lim->io_min = SECTOR_SIZE;
> + if (lim->io_min < lim->physical_block_size)
> + lim->io_min = lim->physical_block_size;
> +
> + if (!lim->max_hw_sectors)
> + lim->max_hw_sectors = BLK_SAFE_MAX_SECTORS;
> + if (WARN_ON_ONCE(lim->max_hw_sectors < PAGE_SIZE / SECTOR_SIZE))
> + return -EINVAL;

The WARN_ON_ONCE usage is odd, as it will only ever WARN once for a
specific q, while other queues associated with other drivers may have
the same limit issue. But I suppose if one issue is fixed, then the
other will make itself known...

> +
> + lim->max_hw_sectors = round_down(lim->max_hw_sectors,
> + lim->logical_block_size >> SECTOR_SHIFT);
> +
> + max_hw_sectors = min_not_zero(lim->max_hw_sectors,
> + lim->max_dev_sectors);
> + if (lim->max_user_sectors) {
> + if (lim->max_user_sectors > max_hw_sectors ||
> + lim->max_user_sectors < PAGE_SIZE / SECTOR_SIZE)
> + return -EINVAL;
> + lim->max_sectors = min(max_hw_sectors, lim->max_user_sectors);
> + } else {
> + lim->max_sectors = min(max_hw_sectors, BLK_DEF_MAX_SECTORS_CAP);
> + }
> +
>
> +/**
> + * queue_limits_commit_update - commit an atomic update of queue limits
> + * @q: queue to update
> + * @lim: limits to apply
> + *
> + * Apply the limits in @lim that were obtained from queue_limits_start_update()
> + * and updated by the caller to @q.
> + *
> + * Returns 0 if successful, else a negative error code.
> + */
> +int queue_limits_commit_update(struct request_queue *q,
> + struct queue_limits *lim)
> + __releases(q->limits_lock)
> +{
> + int error = blk_validate_limits(lim);
> +
> + if (!error) {
> + q->limits = *lim;

this is duplicating what blk_alloc_queue() does

> + if (q->disk)
> + blk_apply_bdi_limits(q->disk->bdi, lim);
> + }
> + mutex_unlock(&q->limits_lock);
> + return error;
> +}
> +EXPORT_SYMBOL_GPL(queue_limits_commit_update);
> +
> /**
> * blk_queue_bounce_limit - set bounce buffer limit for queue
> * @q: the request queue for the device
> diff --git a/block/blk.h b/block/blk.h
> index 1ef920f72e0f87..58b5dbac2a487d 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -447,6 +447,7 @@ static inline void bio_release_page(struct bio *bio, struct page *page)
> unpin_user_page(page);
> }
>
> +int blk_validate_limits(struct queue_limits *lim);
> struct request_queue *blk_alloc_queue(int node_id);
>
> int disk_scan_partitions(struct gendisk *disk, blk_mode_t mode);
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index 4a2e82c7971c86..5b5d3b238de1e7 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -473,6 +473,7 @@ struct request_queue {
>
> struct mutex sysfs_lock;
> struct mutex sysfs_dir_lock;
> + struct mutex limits_lock;
>
> /*
> * for reusing dead hctx instance in case of updating
> @@ -861,6 +862,28 @@ static inline unsigned int blk_chunk_sectors_left(sector_t offset,
> return chunk_sectors - (offset & (chunk_sectors - 1));
> }
>
> +/**
> + * queue_limits_start_update - start an atomic update of queue limits
> + * @q: queue to update
> + *
> + * This functions starts an atomic update of the queue limits. It takes a lock
> + * to prevent other updates and returns a snapshot of the current limits that
> + * the caller can modify. The caller must call queue_limits_commit_update()
> + * to finish the update.
> + *
> + * Context: process context. The caller must have frozen the queue or ensured
> + * that there is outstanding I/O by other means.
> + */
> +static inline struct queue_limits
> +queue_limits_start_update(struct request_queue *q)
> + __acquires(q->limits_lock)
> +{
> + mutex_lock(&q->limits_lock);
> + return q->limits;
> +}
> +int queue_limits_commit_update(struct request_queue *q,
> + struct queue_limits *lim);

It ain't so nice that the code for queue_limits_start_update() and
queue_limits_commit_update() pair are in separate files.

> +
> /*
> * Access functions for manipulating queue properties
> */