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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 22 Sep 2020 11:13:14 +0200
From:   Jan Kara <jack@...e.cz>
To:     Christoph Hellwig <hch@....de>
Cc:     Jens Axboe <axboe@...nel.dk>, Song Liu <song@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        Coly Li <colyli@...e.de>, Richard Weinberger <richard@....at>,
        Minchan Kim <minchan@...nel.org>,
        Johannes Thumshirn <Johannes.Thumshirn@....com>,
        Justin Sanders <justin@...aid.com>,
        linux-mtd@...ts.infradead.org, dm-devel@...hat.com,
        linux-block@...r.kernel.org, linux-bcache@...r.kernel.org,
        linux-kernel@...r.kernel.org, drbd-dev@...ts.linbit.com,
        linux-raid@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, cgroups@...r.kernel.org
Subject: Re: [PATCH 07/13] block: lift setting the readahead size into the
 block layer

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@....de>
> Reviewed-by: Johannes Thumshirn <johannes.thumshirn@....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@...e.com>
SUSE Labs, CR