Re: [PATCH 07/13] block: lift setting the readahead size into the block layer

Jan Kara
2020-09-22T09:13:18Z

On Mon 21-09-20 10:07:28, Christoph Hellwig wrote:
> Drivers shouldn't really mess with the readahead size, as that is a VM
> concept. Instead set it based on the optimal I/O size by lifting the
> algorithm from the md driver when registering the disk. Also set
> bdi->io_pages there as well by applying the same scheme based on
> max_sectors.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
...
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index 76a7e03bcd6cac..01049e9b998f1d 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -452,6 +452,8 @@ EXPORT_SYMBOL(blk_limits_io_opt);
> void blk_queue_io_opt(struct request_queue *q, unsigned int opt)
> {
> blk_limits_io_opt(&q->limits, opt);
> + q->backing_dev_info->ra_pages =
> + max(queue_io_opt(q) * 2 / PAGE_SIZE, VM_READAHEAD_PAGES);
> }
> EXPORT_SYMBOL(blk_queue_io_opt);
>
> @@ -628,9 +630,6 @@ void disk_stack_limits(struct gendisk *disk, struct block_device *bdev,
> printk(KERN_NOTICE "%s: Warning: Device %s is misaligned\n",
> top, bottom);
> }
> -
> - t->backing_dev_info->io_pages =
> - t->limits.max_sectors >> (PAGE_SHIFT - 9);
> }
> EXPORT_SYMBOL(disk_stack_limits);

One thing I've noticed is that blk_stack_limits() does not use
blk_queue_io_opt() to set new optimal limit. That means that ra_pages won't
be updated for the new queue. E.g. your DRDB change below will result in
ra_pages not being properly updated AFAICT.

Similarly it isn't clear to me how io_pages would get updated after
blk_stack_limits() updates max_hw_sectors...

Otherwise the patch looks good.

Honza

> diff --git a/drivers/block/drbd/drbd_nl.c b/drivers/block/drbd/drbd_nl.c
> index aaff5bde391506..f8fb1c9b1bb6c1 100644
> --- a/drivers/block/drbd/drbd_nl.c
> +++ b/drivers/block/drbd/drbd_nl.c
> @@ -1360,18 +1360,8 @@ static void drbd_setup_queue_param(struct drbd_device *device, struct drbd_backi
> decide_on_discard_support(device, q, b, discard_zeroes_if_aligned);
> decide_on_write_same_support(device, q, b, o, disable_write_same);
>
> - if (b) {
> + if (b)
> blk_stack_limits(&q->limits, &b->limits, 0);
> -
> - if (q->backing_dev_info->ra_pages !=
> - b->backing_dev_info->ra_pages) {
> - drbd_info(device, "Adjusting my ra_pages to backing device's (%lu -> %lu)\n",
> - q->backing_dev_info->ra_pages,
> - b->backing_dev_info->ra_pages);
> - q->backing_dev_info->ra_pages =
> - b->backing_dev_info->ra_pages;
> - }
> - }
> fixup_discard_if_not_supported(q);
> fixup_write_zeroes(device, q);
> }
--
Jan Kara <jack@suse.com>
SUSE Labs, CR