linux-ext4 - Re: dm: use queue_limits

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]

Message-ID: <20240520150653.GA32461@lst.de>
Date: Mon, 20 May 2024 17:06:53 +0200
From: Christoph Hellwig <hch@....de>
To: Mike Snitzer <snitzer@...nel.org>
Cc: Theodore Ts'o <tytso@....edu>, dm-devel@...ts.linux.dev,
	fstests@...r.kernel.org, linux-ext4@...r.kernel.org,
	regressions@...ts.linux.dev, Christoph Hellwig <hch@....de>,
	linux-block@...r.kernel.org
Subject: Re: dm: use queue_limits_set

On Sun, May 19, 2024 at 01:42:00AM -0400, Mike Snitzer wrote:
> > This being one potential fix from code inspection I've done to this
> > point, please see if it resolves your fstests failures (but I haven't
> > actually looked at those fstests yet _and_ I still need to review
> > commits d690cb8ae14bd and 4f563a64732da further -- will do on Monday,
> > sorry for the trouble):
> 
> I looked early, this is needed (max_user_discard_sectors makes discard
> limits stacking suck more than it already did -- imho 4f563a64732da is
> worthy of revert.

Can you explain why?  This actually makes the original addition of the
user-space controlled max discard limit work.  No I'm a bit doubful
that allowing this control was a good idea, but that ship unfortunately
has sailed.

Short of that, dm-cache-target.c and possibly other
> DM targets will need fixes too -- I'll go over it all Monday):
> 
> diff --git a/drivers/md/dm-thin.c b/drivers/md/dm-thin.c
> index 4793ad2aa1f7..c196f39579af 100644
> --- a/drivers/md/dm-thin.c
> +++ b/drivers/md/dm-thin.c
> @@ -4099,8 +4099,10 @@ static void pool_io_hints(struct dm_target *ti, struct queue_limits *limits)
>  
>  	if (pt->adjusted_pf.discard_enabled) {
>  		disable_discard_passdown_if_not_supported(pt);
> -		if (!pt->adjusted_pf.discard_passdown)
> -			limits->max_discard_sectors = 0;
> +		if (!pt->adjusted_pf.discard_passdown) {
> +			limits->max_hw_discard_sectors = 0;
> +			limits->max_user_discard_sectors = 0;
> +		}

I think the main problem here is that dm targets adjust
max_discard_sectors diretly instead of adjusting max_hw_discard_sectors.
Im other words we need to switch all places dm targets set
max_discard_sectors to use max_hw_discard_sectors instead.  They should
not touch max_user_discard_sectors ever.

This is probably my fault, I actually found this right at the time
of the original revert of switching dm to the limits API, and then
let it slip as the patch was reverted.  That fact that you readded
the commit somehow went past my attention window.